-
Notifications
You must be signed in to change notification settings - Fork 221
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
Fix uppercase lowercase isses #354
Fix uppercase lowercase isses #354
Conversation
I have chosen the first approach, which is to use lowercase keys which will make the crate case insensitive, this will allow the user to use uppercase or lowercase keys but internally the keys can be easily matched and overridden since they are all lowercase. As of the caveat I mentioned here I managed to fix it by matching the fields and the keys for lowercase in the |
bcb1966
to
da78382
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
One suggestion. Mention that keys are case insensitive in the docs.
Thanks. I will add your suggestion to the chore list 😄 |
7cab5d6
to
0a2e8eb
Compare
Done and ready for a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So first of all, thanks for working on this. This is important to get right.
I have some remarks, nothing too critical.
Still, I am not sure whether this is the right way to go. Converting all things to lowercase seems ... I don't know to be honest.
I am willing to merge this (modulo some things that have to be cleared), if we do not find another way... that said, when (if) we re-do the crate (#321) we should get it right from the get-go and support case sensitivity as well as insensitivity (let the user decide). But that's of course for another day, I guess.
1b03bf4
to
0bb4c7a
Compare
tests for key overrides tests for key overrides tests for key overrides
7227e97
to
b2dfb93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was laying around way too long. I'm gonna merge this now, sorry for me taking so long!
There is still a bug (as of 0.13.3).
File:
Env:
Then: |
Update `config` to the latest minor version. The latest version of `config` adds more detailed error information that require updates to snapshot tests. More significantly, environment config variables are no longer case-sensitive[1]. Internally they are down-cased, so we must adjust `test_command_args` to set lowercase environment variables. In addition, the newly added `config::Value::origin()` method exposes the path of config files. [1] rust-cli/config-rs#354
Update `config` to the latest minor version. The latest version of `config` adds more detailed error information and platform-specific file paths that require updates to snapshot tests. More significantly, environment config variables are no longer case-sensitive[1]. Internally they are down-cased, so we must adjust `test_command_args` to set lowercase environment variables. In addition, the newly added `config::Value::origin()` method exposes the path of config files. [1] rust-cli/config-rs#354
Update `config` to the latest minor version. The latest version of `config` adds more detailed error information and platform-specific file paths that require updates to snapshot tests. More significantly, environment config variables are no longer case-sensitive[1]. Internally they are down-cased, so we must adjust `test_command_args` to set lowercase environment variables. In addition, the newly added `config::Value::origin()` method exposes the path of config files. [1] rust-cli/config-rs#354
Update `config` to the latest minor version. The latest version of `config` adds more detailed error information and platform-specific file paths that require updates to snapshot tests. More significantly, environment config variables are no longer case-sensitive[1]. Internally they are down-cased, so we must adjust `test_command_args` to set lowercase environment variables. In addition, the newly added `config::Value::origin()` method exposes the path of config files. [1] rust-cli/config-rs#354
Update `config` to the latest minor version. The latest version of `config` adds more detailed error information and platform-specific file paths that require updates to snapshot tests. More significantly, environment config variables are no longer case-sensitive[1]. Internally they are down-cased, so we must adjust `test_command_args` to set lowercase environment variables. In addition, the newly added `config::Value::origin()` method exposes the path of config files. [1] rust-cli/config-rs#354
This reverts commit 2ec9497. We do unfortunately have to downgrade since `config 0.14.0` [0] converts all configuration keys to lowercase but some of our configuration keys, e.g. the environment variables, are case sensitive. More details are in the relevant upstream PR [1] that made the crate case insensitive by converting all the keys to lowercase "internally" and a new issue discussing this change/regression [2]. Unfortunately this wasn't really mentioned as a breaking change in the upstream config-rs changelog so I missed it. This fixes such errors: ``` Error: build command failed Caused by: 0: Checking allowed variable names 1: Checking allowed variables for package tree 1.8.0 2: Environment variable name not allowed: additional_make_flags ``` I didn't notice this regression at first as I relied on the tests but they don't catch such errors yet. I'll try to add a test case for this error and check the environment variables defined in `pkg.toml` files during the loading of the repository (should yield better error messages and we do want to verify **all** packages and during the loading of the repository ("evaluation")). [0]: https://github.com/mehcode/config-rs/blob/0.14.0/CHANGELOG.md#0140---2024-02-01 [1]: rust-cli/config-rs#354 [2]: rust-cli/config-rs#531
fix #340