Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

override_file and default_file can be something is not a file #193

Open
Sbozzolo opened this issue May 20, 2024 · 2 comments
Open

override_file and default_file can be something is not a file #193

Sbozzolo opened this issue May 20, 2024 · 2 comments

Comments

@Sbozzolo
Copy link
Member

"""
create_toml_dict(FT;
override_file,
default_file,
)
Creates a `ParamDict{FT}` struct, by reading and merging upto
two TOML files or Julia Dicts with override information taking precedence over
default information.
"""
function create_toml_dict(
::Type{FT};
override_file::Union{Nothing, String, Dict} = nothing,
default_file::Union{String, Dict} = joinpath(@__DIR__, "parameters.toml"),
) where {FT <: AbstractFloat}
default_dict =
default_file isa String ? TOML.parsefile(default_file) : default_file
default_toml_dict = ParamDict{FT}(default_dict, nothing)
isnothing(override_file) && return default_toml_dict
override_dict =
override_file isa String ? TOML.parsefile(override_file) : override_file
override_toml_dict = ParamDict{FT}(override_dict, override_dict)
return merge_override_default_values(override_toml_dict, default_toml_dict)
end

This function has a really counter-intuitive behavior, where you can pass a dictionary to an override_file/default_file. If we want to support this feature, it should be in different method instead of having a function that does all sorts of stuff and that can only be understood by reading the source code.

@nefrathenrici
Copy link
Member

Would it be clearer if the kwargs were changed? It is useful to be able to mix files and Dicts, so I would like to keep this functionality

@Sbozzolo
Copy link
Member Author

I think that an argument that takes nothing, string, and dict will always be a little confusing.

Maybe you could do this:

  1. you have a method that only takes dictionaries (dictionaries are the "primitive type", file reading can be reduced to this case)
  2. you a methods that just call 1. with the input read from file
create_toml_dict(; override_file::String, default_file::String) = create_toml_dict(; override_dict = Toml.parse(override_file), default_dict = Toml.parse(default_file)
create_toml_dict(; override_dict::Dict, default_file::String) = create_toml_dict(; default_dict = Toml.parse(default_file), override_dict)
create_toml_dict(; override_file::String, default_dict::Dict) = create_toml_dict(; default_dict, override_dict = Toml.parse(override_file)

Or something like this.

More generally, to me this looks like a problem with the constructor of ParamDict. At this end, this just is just a constructor, so maybe we should just provide the correct constructors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants