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

Add hidden --extension to override inference of source type from file extension #8373

Merged

Conversation

felix-cw
Copy link
Contributor

@felix-cw felix-cw commented Oct 31, 2023

Summary

This PR addresses the incompatibility with jupyterlab-lsp + python-lsp-ruff arising 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

  • python
  • pyi
  • ipynb

Usage:

ruff check --no-cache --stdin-filename Untitled.ipynb --extension ipynb:python

Unlike the original suggestion, : instead of = is used to associate file extensions to language since that is what is used with --per-file-ignores which is an existing option that accepts a mapping.

Test Plan

2 tests added to integration_test.rs to ensure the override works as expected

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no linter changes.

instead of relying on clap::ValueEnum
@charliermarsh charliermarsh self-requested a review November 6, 2023 01:03
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.

Thank you! I'm comfortable with this approach but can we try moving extension onto settings?

crates/ruff_cli/src/args.rs Outdated Show resolved Hide resolved
crates/ruff_cli/src/commands/check.rs Outdated Show resolved Hide resolved
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! And sorry for the back and forth in the issue :)

crates/ruff_cli/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/ruff_cli/src/diagnostics.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/settings/types.rs Show resolved Hide resolved
return None;
};
match extension.get(ext) {
Some(Language::Python) => Some(PySourceType::Python),
Copy link
Member

Choose a reason for hiding this comment

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

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.

impl AsMode for PySourceType {
fn as_mode(&self) -> Mode {
match self {
PySourceType::Python | PySourceType::Stub => Mode::Module,
PySourceType::Ipynb => Mode::Ipython,
}
}
}

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

github-actions bot commented Nov 7, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

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.

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 :)

@charliermarsh charliermarsh added the cli Related to the command-line interface label Nov 7, 2023
@dhruvmanila dhruvmanila changed the title Add hidden --extension-override to override inference of source type from file extension Add hidden --extension to override inference of source type from file extension Nov 8, 2023
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thanks @felix-cw for contributing and welcome to the repo!

@dhruvmanila dhruvmanila merged commit 7391f74 into astral-sh:main Nov 8, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants