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

Respect #(deprecated) attribute in configuration options #8035

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Oct 18, 2023

Summary

This PR extends the OptionsMetadata derive macro to respect the deprecated attribute.

Adding the deprecated attribute to an option does:

  • Mark the option as deprecated in the json schema (handled by schemars)
  • Adds a deprecated message to the documentation
  • Shows a (deprecated) label when listing the configurations with ruff config
  • Shows the deprecation message when showing an option with ruff config <name>

deprecated is not yet supported for groups.

Test Plan

 cargo run --bin ruff -q -- config extend-ignore
A list of rule codes or prefixes to ignore, in addition to those
specified by `ignore`.

Default value: []
Type: list[RuleSelector]
Deprecated: This option has been **deprecated** in favor of `ignore` since its usage is now interchangeable with `ignore`.
Example usage:
..
cargo run --bin ruff -q -- config 
cache-dir
extend
...
extend-ignore (deprecated)
Screenshot 2023-10-19 at 09 57 21

@MichaReiser MichaReiser added the configuration Related to settings and configuration label Oct 18, 2023
@MichaReiser MichaReiser added the documentation Improvements or additions to documentation label Oct 18, 2023
@@ -212,39 +233,109 @@ struct FieldAttributes {
example: String,
}

impl Parse for FieldAttributes {
fn parse(input: ParseStream) -> syn::Result<Self> {
let default = _parse_key_value(input, "default")?;
Copy link
Member Author

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.

@@ -1662,6 +1695,17 @@
"$ref": "#/definitions/RuleSelector"
}
},
"extend-unfixable": {
Copy link
Member Author

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.

@MichaReiser MichaReiser changed the title Respect deprecated attribute in configuration options Respect #(deprecated) attribute in configuration options Oct 18, 2023
@MichaReiser MichaReiser marked this pull request as ready for review October 18, 2023 06:17
@MichaReiser MichaReiser force-pushed the formatter-respect-tab-size branch from a583805 to f1b00ca Compare October 18, 2023 06:30
Copy link
Member

@charliermarsh charliermarsh left a 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.

crates/ruff_dev/src/generate_options.rs Outdated Show resolved Hide resolved
Base automatically changed from formatter-respect-tab-size to main October 18, 2023 23:48
@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@MichaReiser
Copy link
Member Author

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.

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:

  • Both the new and deprecated options are in the json schema. The deprecated option must be marked as deprecated in the schema.
  • Ideally: The deprecated name is listed next to the new configuration name.

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, schemars needs two fields to have both show up in the schema.

We could remove the option attribute from the deprecated option and add a deprecated_alias field to the option attribute. But it felt like too much complexity for the few deprecated options that we have today.

@MichaReiser MichaReiser enabled auto-merge (squash) October 19, 2023 00:59
@MichaReiser MichaReiser merged commit a85ed30 into main Oct 19, 2023
15 checks passed
@MichaReiser MichaReiser deleted the options-deprecated branch October 19, 2023 01:07
@henryiii
Copy link
Contributor

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)

@zanieb
Copy link
Member

zanieb commented Oct 19, 2023

Thanks for the notes! We'll see at SchemaStore/schemastore#3332

@MichaReiser
Copy link
Member Author

Interesting, I assumed that this is well supported, considering that 2020-12 is the most recent json schema specification

@henryiii
Copy link
Contributor

henryiii commented Oct 19, 2023

It would be nice if the description in JSONSchema would include the deprecation reason. I wasn’t aware that extend-ignore was deprecated and the schema didn’t say why.

@henryiii
Copy link
Contributor

henryiii commented Oct 19, 2023

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

@MichaReiser
Copy link
Member Author

MichaReiser commented Oct 19, 2023

It would be nice if the description in JSONSchema would include the deprecation reason. I wasn’t aware that extend-ignore was deprecated and the schema didn’t say why.

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 deprecated field is a boolean and doesn't allow specifying a message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants