-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ruff
] Update schemars to v1
#20942
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
[ruff
] Update schemars to v1
#20942
Conversation
128d5b2
to
2ce146a
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
2ce146a
to
66af9fe
Compare
|
66af9fe
to
2244ab2
Compare
ruff.schema.json
Outdated
@@ -1415,7 +1405,7 @@ | |||
"null" | |||
], | |||
"items": { | |||
"type": "string" | |||
"$ref": "#/$defs/string" |
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.
These changes looks weird, but they refer to Name
struct, which means String in schemars. That is why they use defs/string
instead of string
.
ruff/crates/ruff_workspace/src/options.rs
Lines 1970 to 1977 in c424007
/// Additional names to ignore when considering `flake8-self` violations, | |
/// in addition to those included in [`ignore-names`](#lint_flake8-self_ignore-names). | |
#[option( | |
default = r#"[]"#, | |
value_type = "list[str]", | |
example = r#"extend-ignore-names = ["_base_manager", "_default_manager", "_meta"]"# | |
)] | |
pub extend_ignore_names: Option<Vec<Name>>, |
ruff/crates/ruff_workspace/src/options.rs
Lines 1960 to 1968 in c424007
/// A list of names to ignore when considering `flake8-self` violations. | |
#[option( | |
default = r#"["_make", "_asdict", "_replace", "_fields", "_field_defaults", "_name_", "_value_"]"#, | |
value_type = "list[str]", | |
example = r#" | |
ignore-names = ["_new"] | |
"# | |
)] | |
pub ignore_names: Option<Vec<Name>>, |
ruff/crates/ruff_python_ast/src/name.rs
Lines 204 to 227 in c424007
#[cfg(feature = "schemars")] | |
impl schemars::JsonSchema for Name { | |
fn is_referenceable() -> bool { | |
String::is_referenceable() | |
} | |
fn schema_name() -> String { | |
String::schema_name() | |
} | |
fn schema_id() -> std::borrow::Cow<'static, str> { | |
String::schema_id() | |
} | |
fn json_schema(generator: &mut schemars::r#gen::SchemaGenerator) -> schemars::schema::Schema { | |
String::json_schema(generator) | |
} | |
fn _schemars_private_non_optional_json_schema( | |
generator: &mut schemars::r#gen::SchemaGenerator, | |
) -> schemars::schema::Schema { | |
String::_schemars_private_non_optional_json_schema(generator) | |
} | |
ruff.schema.json
Outdated
@@ -1185,7 +1175,7 @@ | |||
"null" | |||
], | |||
"items": { | |||
"type": "string" | |||
"$ref": "#/$defs/string" |
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.
Same as below.
"string": { | ||
"type": "string" |
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 is related to #20942 (comment)
ruff.schema.json
Outdated
} | ||
] | ||
}, | ||
"string": { | ||
"type": "string" |
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 is related to #20942 (comment).
ty.schema.json
Outdated
"description": "The root of the project, used for finding first-party modules.\n\nIf left unspecified, ty will try to detect common project layouts and initialize `src.root` accordingly:\n\n* if a `./src` directory exists, include `.` and `./src` in the first party search path (src layout or flat)\n* if a `./<project-name>/<project-name>` directory exists, include `.` and `./<project-name>` in the first party search path\n* otherwise, default to `.` (flat layout)\n\nBesides, if a `./tests` directory exists and is not a package (i.e. it does not contain an `__init__.py` file),\nit will also be included in the first party search path.", | ||
"anyOf": [ | ||
{ | ||
"$ref": "#/$defs/RelativePathBuf" |
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.
With Schemars 1.0 (defaulting to JSON Schema 2020-12), named newtypes are emitted as reusable $ref
s in #/$defs
by default, even when #[serde(transparent)]
makes their schema equal to the inner field. This preserves type identity and deduplicates shared shapes.
ruff/crates/ty_project/src/metadata/value.rs
Lines 330 to 332 in c424007
#[serde(transparent)] | |
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] | |
pub struct RelativePathBuf(RangedValue<SystemPathBuf>); |
"SystemPathBuf": { | ||
"description": "An owned, mutable path on [`System`](`super::System`) (akin to [`String`]).\n\nThe path is guaranteed to be valid UTF-8.", | ||
"type": "string" | ||
}, |
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 is related to #20942 (comment).
4cabd1e
to
7c5b4d4
Compare
ty.schema.json
Outdated
{ | ||
"type": "string", | ||
"pattern": "^\\d+\\.\\d+$" | ||
"pattern": "^\\\\d+\\\\.\\\\d+$" |
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 escaping here looks incorrect to me
crates/ruff_python_ast/src/name.rs
Outdated
|
||
fn schema_name() -> String { | ||
String::schema_name() | ||
fn schema_name() -> std::borrow::Cow<'static, str> { |
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 we could replace this with
#[cfg_attr(
feature = "serde",
derive(serde::Serialize, serde::Deserialize),
serde(transparent)
)]
#[cfg_attr(feature = "cache", derive(ruff_macros::CacheKey))]
#[cfg_attr(feature = "salsa", derive(salsa::Update))]
#[cfg_attr(feature = "get-size", derive(get_size2::GetSize))]
#[cfg_attr(
feature = "schemars",
derive(schemars::JsonSchema),
schemars(with = "String")
)]
(serde(transparent) and schemars(with="String")
)
let string_with_pattern = schemars::json_schema!({ | ||
"type": "string", | ||
"pattern": r"^\\d+\\.\\d+$", | ||
}); | ||
|
||
let mut any_of: Vec<Value> = Vec::new(); | ||
any_of.push(string_with_pattern.into()); |
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.
Nit
let string_with_pattern = schemars::json_schema!({ | |
"type": "string", | |
"pattern": r"^\\d+\\.\\d+$", | |
}); | |
let mut any_of: Vec<Value> = Vec::new(); | |
any_of.push(string_with_pattern.into()); | |
let mut any_of: Vec<Value> = vec![ | |
schemars::json_schema!({ | |
"type": "string", | |
"pattern": r"^\\d+\\.\\d+$", | |
}) | |
]; |
crates/ruff_workspace/src/options.rs
Outdated
per_file_ignores: Option<BTreeMap<String, Vec<RuleSelector>>>, | ||
extend_per_file_ignores: Option<BTreeMap<String, Vec<RuleSelector>>>, |
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.
What's the reason for changing those fields to a BTreeMap
?
} | ||
} | ||
|
||
#[cfg(feature = "schemars")] |
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.
Nit: I'd prefer to keep this in its own mod
(and in the same position, it makes attributing changes with git easier)
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.
What I want to do is implementing JsonSchema for Rules instead of schemars(with = "schema::Rules")
, so I will keep the mod and implement it inside the module.
// Promote well-known values for better auto-completion. | ||
// Using `const` over `enumValues` as recommended [here](https://github.com/SchemaStore/schemastore/blob/master/CONTRIBUTING.md#documenting-enums). |
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.
Can we preserve this comment
ruff.schema.json
Outdated
@@ -1,15 +1,15 @@ | |||
{ | |||
"$schema": "http://json-schema.org/draft-07/schema#", | |||
"$schema": "https://json-schema.org/draft/2020-12/schema", |
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.
SchemaStore's recommendation is to still use draft-07 because many editors lack support for newer drafts, see
We recommend using the draft-07 JSON schema version. Later versions of JSON Schema are not yet recommended for use in SchemaStore until IDE and language support improves for those versions.
https://github.com/SchemaStore/schemastore/blob/master/CONTRIBUTING.md#best-practices
That's why I think we should not change the schema version as part of this PR
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.
Ok. It's also possible to continue to use draft-07. I will replace the schema version.
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.
Thank you. This looks great overall. My only concern is the bump of the schema version. SchemaStore recommends draft-07 for best editor compatibility. I suggest we defer the schema version bump as it requires extensive testing across editors.
065b5e4
to
7711a64
Compare
@MichaReiser Thank you for the review. I have addressed your comments! |
This is great. Thank you so much! |
Summary
FIxes #19173
Update schemars to v1
I’ve verified that the fundamentals of ruff.schema.json and ty.schema.json remain unchanged following the Schemars upgrade.
The migration is based on https://graham.cool/schemars/migrating.
Shared Changes
Upgraded both schemas to JSON Schema draft 2020-12 andmigrated shared types into$defs
, with all$ref
paths now using#/$defs/...
.$defs/string
(and array helpers likeArray_of_string
) to avoid duplicating scalar/collection shapes.description
verbatim. ref: Handling of newline in RustDoc GREsau/schemars#120ty.schema.json
$defs
entries for path-related types (RelativePathBuf
,SystemPathBuf
) and updated path properties to reference them.OverridesOptions
array definition with expanded multi-line documentation and examples, while maintaining the original rule descriptions under the updated formatting.Test Plan
Check if existing tests pass.