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

JSON output on ruff check --fix does not show any thing related to fixes #11329

Open
inoa-jboliveira opened this issue May 8, 2024 · 9 comments
Labels
question Asking for support or clarification

Comments

@inoa-jboliveira
Copy link

ruff 0.4.3 (1e91a09 2024-05-03)

I am trying to parse the output of ruff check --fix --fix-only --show-fixes to validate what has been changed. When using output = text or full, I get:

- my_file.py:
    1 × E703 (useless-semicolon)

That is byproduct of the "--show-fixes", but I think this format is not very stable as it has changed before and also it has a non ascii character that prints weird in some places.

I would expect to get something meaningful out of json output when using show-fixes, but the process output is always empty (not even the [] from checks since I use --fix-only)

I have tried all commands with --isolated and it behaves the same.

@inoa-jboliveira inoa-jboliveira changed the title Json output on ruff check --fix does not show any thing related to fixes JSON output on ruff check --fix does not show any thing related to fixes May 8, 2024
@inoa-jboliveira inoa-jboliveira changed the title JSON output on ruff check --fix does not show any thing related to fixes JSON output on ruff check --fix does not show any thing related to fixes May 8, 2024
@dhruvmanila
Copy link
Member

You can extract the fix from the JSON output directly without providing any other flags:

$ ruff check --output-format=json src/lsp.py
[
  {
    "cell": null,
    "code": "F401",
    "end_location": {
      "column": 25,
      "row": 3
    },
    "filename": "/Users/dhruv/playground/ruff/src/lsp.py",
    "fix": {
      "applicability": "safe",
      "edits": [
        {
          "content": "",
          "end_location": {
            "column": 1,
            "row": 4
          },
          "location": {
            "column": 1,
            "row": 3
          }
        }
      ],
      "message": "Remove unused import: `pathlib.Path`"
    },
    "location": {
      "column": 21,
      "row": 3
    },
    "message": "`pathlib.Path` imported but unused",
    "noqa_row": 3,
    "url": "https://docs.astral.sh/ruff/rules/unused-import"
  },
  {
    "cell": null,
    "code": "T201",
    "end_location": {
      "column": 6,
      "row": 5
    },
    "filename": "/Users/dhruv/playground/ruff/src/lsp.py",
    "fix": {
      "applicability": "unsafe",
      "edits": [
        {
          "content": "",
          "end_location": {
            "column": 1,
            "row": 12
          },
          "location": {
            "column": 1,
            "row": 5
          }
        }
      ],
      "message": "Remove `print`"
    },
    "location": {
      "column": 1,
      "row": 5
    },
    "message": "`print` found",
    "noqa_row": 5,
    "url": "https://docs.astral.sh/ruff/rules/print"
  }
]

Does this help?

@dhruvmanila dhruvmanila added the question Asking for support or clarification label May 8, 2024
@inoa-jboliveira
Copy link
Author

Hi @dhruvmanila, thank you for the reply.
Then I have to run the code twice, one to find all issues in files that are fixable and a second time to actually fix them.

I believe this feature should already work but some combination of --fix, --show-fixes and --fix-only prevents anything from being output.

@dhruvmanila
Copy link
Member

Do you mean that you want the JSON output and that Ruff applies the fix?

@inoa-jboliveira
Copy link
Author

Yes, correct.

If the information is available in one output type I think it makes sense it is available in all of them.

Otherwise --show-fixes is useless with json output

@dhruvmanila
Copy link
Member

Thanks for confirming!

I think this is an ideal behavior although it's not enforced on the CLI. This is because --output-format flag shouldn't affect the format of --show-* flags which themselves have their own output format (e.g., --show-files, --show-settings).

Can you provide the use-case for this behavior? What kind of information are you looking for with the applied fixes?

@inoa-jboliveira
Copy link
Author

This is because --output-format flag shouldn't affect the format of --show-* flags which themselves have their own output format

Well, it does affect right now because JSON output prevents --show-fixes from displaying anything

Can you provide the use-case for this behavior?

Right now I am trying to display to users what have been changed on their code. This is part of a pre-commit script

@dhruvmanila
Copy link
Member

Right now I am trying to display to users what have been changed on their code. This is part of a pre-commit script

Thank you for providing your use-case!

Well, it does affect right now because JSON output prevents --show-fixes from displaying anything

I see. Looking at the following code:

ruff/crates/ruff/src/lib.rs

Lines 295 to 301 in 1f79407

let mut printer_flags = PrinterFlags::empty();
if !(cli.diff || fix_only) {
printer_flags |= PrinterFlags::SHOW_VIOLATIONS;
}
if show_fixes {
printer_flags |= PrinterFlags::SHOW_FIX_SUMMARY;
}

Ruff would always prefer to display violations unless either the --diff or --fix-only flag was supplied.

And as per the following code:

if matches!(
self.format,
SerializationFormat::Text
| SerializationFormat::Full
| SerializationFormat::Concise
| SerializationFormat::Grouped
) {
if self.flags.intersects(Flags::SHOW_FIX_SUMMARY) {
if !diagnostics.fixed.is_empty() {
writeln!(writer)?;
print_fix_summary(writer, &diagnostics.fixed)?;
writeln!(writer)?;
}
}
self.write_summary_text(writer, diagnostics)?;
}
return Ok(());

Ruff wouldn't display the fix summary if the output format isn't either "full", "concise", or "grouped".

So, I think this behavior is correct because if someone wants to output in JSON, it's likely to be post-processed where it wouldn't make sense to provide additional output including the fix summary.

I would expect to get something meaningful out of json output when using show-fixes, but the process output is always empty (not even the [] from checks since I use --fix-only)

I think this is an expected behavior. The JSON output should only include the violations that are leftover after the fixes has been applied. It wouldn't include the violations which has been fixed.

tl;dr

  • The --output-format=json is for changing the format of the detected violations. For fixes, these are the remaining violations after the fixes have been applied.
  • The --show-fixes is to append a summary of the fixed violations after the diagnostic output but if output format is JSON, it's suppressed
  • The --fix-only is to only apply the fix which means no diagnostics will be displayed

(Sorry for the long explanation, but it's helpful to know the context.)

@inoa-jboliveira
Copy link
Author

inoa-jboliveira commented May 10, 2024

The JSON output should only include the violations that are leftover after the fixes has been applied.

I understood why it is happening, but I don't think I agree this is expected behavior. There is nowhere saying "--show-fixes" should only append information to the output and not be part of the normal output

It would make much more sense if it modified the output to contain "fixed": true on the violations as opposed to not show anything when json or other formats meant for ingestion.

with --fix and --show-fixes it would be something like this (hopefully)

[
   {"code": "F401", ... , "fixed": false},
   {"code": "E703", ... , "fixed": true},
]

with --fix and --show-fixes and --fix-only:

[
   {"code": "E703", ... , "fixed": true},
]

@charliermarsh
Copy link
Member

I'm a bit torn. It could make sense to include the fixed violations but we'd of course be changing the meaning of the API (right now, the contract is: these are the violations that Ruff found that exist in your files). Including fixed violations is a bit strange because (e.g.) the locations no longer make sense, since the file has changed.

I'd probably be open to changing it (curious to hear @dhruvmanila take) but wouldn't prioritize it unless someone wanted to take it on. I don't think we even have the information we'd need to power this in reporter right now.

We could also consider using an entirely different JSON output for --show-fixes where we just write the existing output but structured as JSON.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Asking for support or clarification
Projects
None yet
Development

No branches or pull requests

3 participants