-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Respect #(deprecated) attribute in configuration options
#8035
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
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
0e630f3 to
bdcf7c1
Compare
|
|
||
| impl Parse for FieldAttributes { | ||
| fn parse(input: ParseStream) -> syn::Result<Self> { | ||
| let default = _parse_key_value(input, "default")?; |
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.
The old logic relied on the specific order. The new logic allows arbitrary ordering and is in line with that serde and other crates use for parsing attribute fields.
| "$ref": "#/definitions/RuleSelector" | ||
| } | ||
| }, | ||
| "extend-unfixable": { |
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.
We shouldn't remove deprecated options from the schema to avoid existing configurations failing the schema validation.
deprecated attribute in configuration options#(deprecated) attribute in configuration options
a583805 to
f1b00ca
Compare
bdcf7c1 to
f2ac348
Compare
charliermarsh
left a comment
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.
When we rename an option, will we leave the deprecated variant and mark it as deprecated? I assume both will then show up in the documentation.
f2ac348 to
03e903d
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
Yes, marking an option as deprecated has the result that both, the new and the deprecated, options show up in the documentation. We can customize the layout for deprecated options if we want. I considered alternatives for the special case of renaming options but they all require more tooling support which I'm not sure is worth building. What I want is that:
The challenge is that we need two fields to know if the user used the deprecated field (we could create a custom deserializer and emit the warning in the deserializer but it will be challenging to ever return this warning as a diagnostic in the future). Also, We could remove the |
03e903d to
d601bfd
Compare
d601bfd to
f5ddc5a
Compare
|
Curious to see how SchemaStore (and other validators, like fastjsonschema) handle this - might need to be added to the ignored keyword list in their internal validator (which is easy to do per schema). Technically, it was added in draft 2019, which pretty much nothing seems to officially support. But like other features, I don't think extra keywords are disallowed, and validators may start picking up on this (besides schemas, I know there's discussion on making jsonschema in Python support it). (This is my favorite 2019 feature, though, and I'd love to see it added everywhere) |
|
Thanks for the notes! We'll see at SchemaStore/schemastore#3332 |
|
Interesting, I assumed that this is well supported, considering that 2020-12 is the most recent json schema specification |
|
It would be nice if the description in JSONSchema would include the deprecation reason. I wasn’t aware that |
|
Most tools seem to have frozen at draft 7. Fastjsonschema (used by validate-pyproject) requires 7. Even schemastore requires 7: https://github.com/SchemaStore/schemastore/blob/master/CONTRIBUTING.md#best-practices |
Yeah. I haven't figured out a good (technical) solution to this yet which doesn't require us to re-implement schemar's derive macro other than copying the deprecation message into the description. It's a pity that the json schema |

Summary
This PR extends the
OptionsMetadataderive macro to respect thedeprecatedattribute.Adding the
deprecatedattribute to an option does:deprecatedmessage to the documentation(deprecated)label when listing the configurations withruff configruff config <name>deprecatedis not yet supported for groups.Test Plan