Use hardcoded filenames. #815
Conversation
core/src/config.jl
Outdated
| basin::String = "basin.arrow" | ||
| flow::String = "flow.arrow" | ||
| control::String = "control.arrow" | ||
| allocation::String = "allocation.arrow" |
There was a problem hiding this comment.
To fit #799 (comment)
Those fields need to be removed as far as I can see
There was a problem hiding this comment.
These fields need to be hardcodes somewhere for core to write them.
There was a problem hiding this comment.
Sure, but if you hardcode them here, it will be possible to override them with the TOML right?
There was a problem hiding this comment.
I now always override them.
python/ribasim/ribasim/config.py
Outdated
| basin: Path = Field(default=Path("basin.arrow"), exclude=True, repr=False) | ||
| flow: Path = Field(default=Path("flow.arrow"), exclude=True, repr=False) | ||
| control: Path = Field(default=Path("control.arrow"), exclude=True, repr=False) | ||
| allocation: Path = Field(default=Path("allocation.arrow"), exclude=True, repr=False) |
There was a problem hiding this comment.
These need to be removed too
There was a problem hiding this comment.
I've excluded them from any exports. They are here for when we need to parse the output files.
There was a problem hiding this comment.
Just an FYI: I just found out that TOML hates these Path objects, and complains that it cannot serialize them.
However, due to the exclude_unset logic, they aren't included unless you set them yourself. For extra confusion, if you set the path (in current main) as a string, pydantic will coerce it to a Path. Then pydantic detects it has been set, and then TOML complains that it cannot serialize it.
To be honest, together with issue #811, I'm getting really radicalized against exclude_unset.
There was a problem hiding this comment.
That's why there's
@field_serializer("input_dir", "results_dir")
def serialize_path(self, path: Path) -> str:
return str(path)
Right? Which fields are you trying to set?
I don't like the unset behaviour either, but that should be a separate issue/discussion.
- Wrap `Config` around new struct `Toml` - Remove result keys: `basin`, `flow`, `control` and `allocation` - Create constants for these keys - Remove default values of `input_dir` and `results_dir`
Hofer-Julian
force-pushed
the
feat/hardcoded-filenames
branch
from
1440df8 to
ca869bb
Compare
core/src/config.jl
Outdated
| struct Config | ||
| toml::Toml | ||
| relative_dir::String | ||
| end |
There was a problem hiding this comment.
I like separating the TOML from other things. I don't like having to always type e.g. config.toml.starttime. Would it work for you if we forward Base.getproperty on Config to the toml field? To maintain the old API. To access the relative_dir field, which perhaps should just be dir or directory, we could add Base.dirname(config::Config) = getfield(config, :relative_dir).
The Base.show method in this file probably also need updating.
There was a problem hiding this comment.
forward Base.getproperty on Config to the toml field
Nice idea. It adds a bit complexity, but I think it's worth it.
relative_dir field, which perhaps should just be dir or directory
I think relative_dir is more descriptive
we could add Base.dirname(config::Config) = getfield(config, :relative_dir)
The docs of Base.dirname talk about paths.
Since config isn't a path, I don't think it is applicable here. I've created a function relative_dir instead.
The Base.show method in this file probably also need updating.
It seems to work fine for me. It's what is called if I evaluate a Config instance in the REPL, right?
There was a problem hiding this comment.
Thanks! Not sure what you think, but the name relative_dir has always been a bit confusing to me. It is not necessarily relative, it is just a directory, it's other paths in the TOML that are relative to it.
Regarding Base.dirname, good that you checked the docstring for it. The line "Get the directory part of a path." really seems to describe the particular ::String method though. If it is a true function docstring it should describe what is does ("get the directory"), not from where. And what it does seems to match fine. Wflow also has this method on their Config.
Indeed Base.show seems to work fine, I mistakenly assumed it would be broken by this PR.
core/src/config.jl
Outdated
| struct Config | ||
| toml::Toml | ||
| relative_dir::String | ||
| end |
There was a problem hiding this comment.
Thanks! Not sure what you think, but the name relative_dir has always been a bit confusing to me. It is not necessarily relative, it is just a directory, it's other paths in the TOML that are relative to it.
Regarding Base.dirname, good that you checked the docstring for it. The line "Get the directory part of a path." really seems to describe the particular ::String method though. If it is a true function docstring it should describe what is does ("get the directory"), not from where. And what it does seems to match fine. Wflow also has this method on their Config.
Indeed Base.show seems to work fine, I mistakenly assumed it would be broken by this PR.
Co-authored-by: Martijn Visser <[email protected]>
|
@visr I've renamed |
This code gives an error in the QGIS plugin:
```python
def _get_database_path_from_model_file(self) -> str:
with open(self.path, "rb") as f:
model_filename = tomllib.load(f)["database"]
return str(Path(self.path).parent.joinpath(model_filename))
```
Because the database entry is no longer present in the TOML because of
#815.
Fixes #799