Add hidden --extension to override inference of source type from file extension#8373
Conversation
PR Check ResultsEcosystem✅ ecosystem check detected no linter changes. |
instead of relying on clap::ValueEnum
charliermarsh
left a comment
There was a problem hiding this comment.
Thank you! I'm comfortable with this approach but can we try moving extension onto settings?
dhruvmanila
left a comment
There was a problem hiding this comment.
Thanks for doing this! And sorry for the back and forth in the issue :)
crates/ruff_cli/src/diagnostics.rs
Outdated
| return None; | ||
| }; | ||
| match extension.get(ext) { | ||
| Some(Language::Python) => Some(PySourceType::Python), |
There was a problem hiding this comment.
Sorry for not noticing this in my first review 😓
How will this work if the cell contained IPython escape commands (!pwd, %matplotlib inline)? The parser mode is tied up with PySourceType. This mode is then used to allow the escape commands.
ruff/crates/ruff_python_parser/src/lib.rs
Lines 325 to 332 in 2d5ce45
Now, if the cell contained escape commands and the override flag tells that this is a Python language (--extension-override ipynb:python), then it'll throw a syntax error.
There was a problem hiding this comment.
jupyterlab-lsp can replace notebook magics with equivalent python code in most cases, so in this use case these syntax errors are avoided (I think the code is here).
Any python code affected by magic ends up in a string so doesn't get checked, for example with magics like %time and %%time.
There was a problem hiding this comment.
Thanks for letting us know about that.
Coming from a public API perspective, this might still be a problem as, for example, if someone's trying to use it in a similar manner with an integration that doesn't perform those transformations, we'd throw an error.
I would love any other opinion on this. I'm fine with including the flag but if we include it in the config, then it's part of the public API which is where I'm unsure of.
There was a problem hiding this comment.
FWIW jupyter nbconvert --to script also seems to convert notebook magics to python in the same way. nbdev has a routine callled scrub_magics which removes magics altogether, though it seems to be optional.
The docs could make it clear that python means python without notebookisms. Maybe something like:
Users who use this option to lint code extracted from notebooks as python should ensure that the code should be free of IPython magics, as these will cause a syntax error.
|
charliermarsh
left a comment
There was a problem hiding this comment.
I pushed some nits, and I'll let @dhruvmanila approve and merge ultimately, but this looks good to me. I opted to remove the setting from Options for now, since @dhruvmanila is right that we only need to support it on the CLI -- but I do like that it's on Settings rather than a standalone argument, so I think that refactor was necessary anyway :)
--extension-override to override inference of source type from file extension--extension to override inference of source type from file extension
dhruvmanila
left a comment
There was a problem hiding this comment.
Thanks @felix-cw for contributing and welcome to the repo!
Summary
This PR addresses the incompatibility with
jupyterlab-lsp+python-lsp-ruffarising from the inference of source type from file extension, raised in #6847.In particular it follows the suggestion in #6847 (comment) to specify a mapping from file extension to source type.
The source types are
Usage:
Unlike the original suggestion,
:instead of=is used to associate file extensions to language since that is what is used with--per-file-ignoreswhich is an existing option that accepts a mapping.Test Plan
2 tests added to
integration_test.rsto ensure the override works as expected