Implement anti-duplicate parsing for config.tools #65
Conversation
During deserialization, ensures that there are no duplicate entries in config.tools. By default, serde will overwrite duplicate entries when deserializing to a HashMap<K, V>. In almost all cases, a user would be incorrectly doing so accidentally, so we now throw an error when parsing the configuration file.
|
The cleanest way I found to do this was to create a wrapper struct around the |
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| .unwrap_err(); | ||
|
|
||
| assert_eq!(err.to_string(), "duplicate tool `tool` at line 1 column 1"); | ||
| } |
There was a problem hiding this comment.
Would you mind adding a test case to document the issue you're having with the line/column number being always 1?
I would like to see something like these cases:
tool_a = { github = "user/a", version = "0.1.0" }
tool_b = { github = "user/b", version = "0.1.0" }
tool_a = { github = "user/c", version = "0.1.0" }tool_b = { github = "user/b", version = "0.1.0" }
tool_a = { github = "user/a", version = "0.1.0" }
tool_a = { github = "user/c", version = "0.1.0" }There was a problem hiding this comment.
Added these test cases now.
❯ cat foreman.toml
[tools]
# oops!
selene = { source = "rojo-rbx/rojo", version = "=7.1.1" }
selene = { source = "Kampfkarren/selene", version = "=0.19.1" }
❯ selene --version
unable to parse Foreman configuration file (at ~/project/foreman.toml): duplicate tool name `selene` found for key `tools` at line 3 column 1
A Foreman configuration file looks like this:
[tools] # list the tools you want to install under this header
# each tool is on its own line, the tool name is on the left
# side of `=` and the right side tells Foreman where to find
# it and which version to download
tool_name = { github = "user/repository-name", version = "1.0.0" }
# tools hosted on gitlab follows the same structure, except
# `github` is replaced with `gitlab`
# Examples:
stylua = { github = "JohnnyMorganz/StyLua", version = "0.11.3" }
darklua = { gitlab = "seaofvoices/darklua", version = "0.7.0" }So I think it's not really an issue, it's just that the line serde points to is where [tools] is in the toml, rather than where the duplicate tool is. Unsure if that is a big enough issue to warrant looking in to.
| #[derive(Debug, Serialize, Deserialize)] | ||
| pub struct ConfigFile { | ||
| pub tools: HashMap<String, ToolSpec>, | ||
| pub tools: ConfigFileTools, |
There was a problem hiding this comment.
If you want, I think a neat tiny little improvement would be to make this field private and add a public method like pub fn tools(&self) -> impl Iterator<Item = &(String, ToolSpec)>
There was a problem hiding this comment.
Since some external files (like src/main.rs) want to perform methods like tools.get, I think it would probably be best to just do
pub fn tools(&self) -> &HashMap<String, ToolSpec> {
&self.tools.0
}which would leave us to have to create a tools_mut as well (as ConfigFile::fill_from needs it), or we could leave it the current way it is (implement Deref and DerefMut).
Co-authored-by: oltrep <[email protected]>
|
Looks like CI is failing as it's using Rust 1.53 which is before named parameters was implemented in 1.58 |
|
Just fixed conflicts with I can bump CI in this PR (or another PR) to Rust |
Thanks for the follow up! Yes let's bump to Rust 1.58 🙂 I'll take a final look after that and merge that in, if you could add an entry to the changelog that would be great |
Allows the support of named parameters, which is required for the anti-duplicate config
During deserialization, ensures that there are no duplicate entries in config.tools. By default, serde will overwrite duplicate entries when deserializing to a HashMap<K, V>.
In almost all cases, a user would be incorrectly doing so accidentally, so we now throw an error when parsing the configuration file.
A unit test has been implemented to test the logic of this deserialization for duplicate tool entries.
will error with
Closes #63