-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
docs: Update --config flag help text #12059
Conversation
This commit changes parsing of configuration file provided with "--config" strict, ie. if the file contains unknown fields it will refuse to parse.
cli/config_file.rs
Outdated
@@ -315,7 +315,7 @@ pub struct FmtConfig { | |||
} | |||
|
|||
#[derive(Clone, Debug, Deserialize)] | |||
#[serde(rename_all = "camelCase")] | |||
#[serde(deny_unknown_fields, rename_all = "camelCase")] |
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.
I'm not sure we should do this yet. Right now --config
is a tsconfig.json which may contain more properties than what we have here. Maybe we should wait for 2.0 when this becomes a deno.json file?
The CLI docs currently say:
-c, --config <FILE> Load tsconfig.json configuration file
Or are we considering this a deno.json file now?
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.
I thought we're gonna eye-ball auto-loading for 1.15 (and thus it would be deno.json
or deno.jsonc
). I'm also a bit skeptical that it might cause some pain. I'm fine going either way.
@ry want to settle this?
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.
I think it's safer to not enable this right now.
I agree with David, let's put this off for a little bit.
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.
I am also for waiting on this, or even not doing it. import-maps and tsconfig.json simply ignore excess properties. I would like to hear a good justification of why we would want to do it.
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.
I am also for waiting on this, or even not doing it. import-maps and tsconfig.json simply ignore excess properties. I would like to hear a good justification of why we would want to do it.
To avoid package.json
situation where different tools start squatting on different keys in that file.
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.
That seems really cruel punishment on the user for no increase in usability.
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.
It was one of goals we agreed upon when designing config file with @lucacasonato and @ry. Unfortunately I don't see it mentioned in any of the issues/design docs.
I believe it's very bad in the long run to allow it, because instead of well-defined format you get a monstrosity with 30+ top level keys used for all various reasons (eg. vscode
extension defining actions in there).
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.
I agree with eventually denying people from squatting to avoid future name collisions with some popular tool that uses a key we want to now use.
I have reverted changes to field strictness, I still would like to land the change to |
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.
LGTM
This commit changes parsing of configuration file provided with
"--config" strict, ie. if the file contains unknown fields it will
refuse to parse.