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

Remove min and max range on line-length JSON schema #7875

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

charliermarsh
Copy link
Member

Summary

This was introduced in #7412, but SchemaStore doesn't accept it. I manually edited the JSON schema last time, then tried to fix this, then gave up -- so removing for now.

(See, e.g., SchemaStore/schemastore#3278, which failed prior to removing the min and max.)

@charliermarsh charliermarsh added the internal An internal refactor or improvement label Oct 9, 2023
@zanieb
Copy link
Member

zanieb commented Oct 9, 2023

Can we not just add exceptions in the schema store schema validator similar to what I did at https://github.com/SchemaStore/schemastore/pull/3221/files#diff-5b41cf0105f054a82fd2932cf8e4af10ec697a315a9fe61d3f1956c0a47cdafc? We're already not producing a schema that passes their strict validation.

I don't think we should remove correct validation on our end if avoidable.

@charliermarsh
Copy link
Member Author

I'll give it a quick try on SchemaStore! If it doesn't work, we should merge this and revisit later. From a procedural perspective, this is a clean revert of a change that broke the release. (Users already aren't benefiting from the change because we can't release the updated schema anyway.)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@charliermarsh
Copy link
Member Author

charliermarsh commented Oct 9, 2023

This is the change we need in the schema file: SchemaStore/schemastore@75d4ec6 (I made it manually). I believe I've tried to get schemars to output this in the past and failed. (I couldn't get our current schema to pass by adding exclusions on the SchemaStore side.) I believe we should merge this in the absence of an owner that wants to prioritize fixing the root cause.

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. My main concern is that the range is relatively restricted and I worry users will encounter invalid values.

It'd be great to open an issue somewhere to track restoring this if we merge without resolving or to explore what happens if users set the length above the support range and how we show errors for this.

My naive test with

❯ ruff check example.py --show-settings | grep line_length -A 2
        line_length: LineLength(
            2000,
        ),

didn't error which surprises me?

edit: okay I did succeed with a larger value

  Cause: Failed to parse `/Users/mz/eng/src/astral-sh/ruff/ruff.toml`: TOML parse error at line 2, column 15
  |
2 | line-length = 1000000
  |               ^^^^^^^
invalid value: integer `1000000`, expected a nonzero u16

@charliermarsh
Copy link
Member Author

Maybe I can try post-processing the JSON. Sounds like a pain in Rust though 😬

@charliermarsh
Copy link
Member Author

Tracking in #7877.

@charliermarsh charliermarsh merged commit d54cabd into main Oct 9, 2023
16 checks passed
@charliermarsh charliermarsh deleted the charlie/line-length branch October 9, 2023 19:36
@henryiii
Copy link
Contributor

henryiii commented Oct 9, 2023

For posterity, this is not SchemaStore being overly strict, it's invalid JSONSchema. This following is invalid:

    "line-length": {
      "description": "The line length to use when enforcing long-lines violations (like `E501`). Must be greater than `0` and less than or equal to `320`.",
      "anyOf": [
        {
          "$ref": "#/definitions/LineLength"
        },
        {
          "type": "null"
        }
      ],
      "maximum": 320.0,
      "minimum": 1.0
    }

An anyOf can't have a minimum/maximum length. Only a "type": "number" has those properties. So it has to go inside the definition at "#/definitions/LineLength".

If schemars is making the definitions directly then I'd guess it's a schemars bug?

@charliermarsh
Copy link
Member Author

Thanks @henryiii, much appreciated!

@@ -362,7 +362,6 @@ pub struct Options {
line-length = 120
"#
)]
#[cfg_attr(feature = "schemars", schemars(range(min = 1, max = 320)))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWICT, it looks like this is applying to the Option<LineLength>, rather than the LineLength inside the Option?

Copy link
Contributor

@henryiii henryiii Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I can replicate it with a MWE:

use schemars::{schema_for, JsonSchema};

#[derive(JsonSchema)]
pub struct MyStruct {
    pub my_int: i32,
}

#[derive(JsonSchema)]
pub struct MyConfig {
    #[schemars(range(min = 1, max = 320))]
    my_struct: Option<MyStruct>,
}

fn main() {
    let schema = schema_for!(MyConfig);
    println!("{}", serde_json::to_string_pretty(&schema).unwrap());
}

And one fix is to move the location of the attribute:

use schemars::{schema_for, JsonSchema};

#[derive(JsonSchema)]
pub struct MyStruct {
    #[schemars(range(min = 1, max = 320))]
    pub my_int: i32,
}

#[derive(JsonSchema)]
pub struct MyConfig {
    my_struct: Option<MyStruct>,
}

fn main() {
    let schema = schema_for!(MyConfig);
    println!("{}", serde_json::to_string_pretty(&schema).unwrap());
}

But not sure how that works from the outer location, and not sure what cfg_attrs is (nevermind, I do). I see an inner(), but it seems to only apply to things like Vector, not to Enums.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I know what that is, and how to fix it (I think).

charliermarsh pushed a commit that referenced this pull request Oct 10, 2023
## Summary

Restores functionality of #7875 but in the correct place. Closes #7877.

~~I couldn't figure out how to get cargo fmt to work, so hopefully
that's run in CI.~~ Nevermind, figured it out.

## Test Plan

Can see output of json.
konstin pushed a commit that referenced this pull request Oct 11, 2023
## Summary

This was introduced in #7412, but
SchemaStore doesn't accept it. I manually edited the JSON schema last
time, then tried to fix this, then gave up -- so removing for now.

(See, e.g., SchemaStore/schemastore#3278, which
failed prior to removing the min and max.)
konstin pushed a commit that referenced this pull request Oct 11, 2023
## Summary

Restores functionality of #7875 but in the correct place. Closes #7877.

~~I couldn't figure out how to get cargo fmt to work, so hopefully
that's run in CI.~~ Nevermind, figured it out.

## Test Plan

Can see output of json.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants