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 support for ruff extension option to enable jupyterlab-lsp #59

Merged
merged 4 commits into from
Nov 25, 2023

Conversation

felix-cw
Copy link
Contributor

@felix-cw felix-cw commented Nov 10, 2023

EDIT: Following the comment below #59 (comment), removed the option for the --extension ipynb:python flag and enabled it in all cases.


Since ruff version 0.0.285, python-lsp-ruff no longer works with jupyterlab-lsp. I previously raised the issue on the ruff project here: astral-sh/ruff#6847. Another user had the same issue and raised it in the jupyterlab-lsp project here: jupyter-lsp/jupyterlab-lsp#1003.

this is because jupyterlab-lsp extracts the python code from the notebook cells, then python-lsp-ruff passes it to ruff via stdin, and specifies --stdin-filename. Since astral-sh/ruff#6628, this makes ruff expect ipynb JSON, rather than python code, which causes the error.

As of ruff version 0.1.5 there is a way to override this. Namely add --extension ipynb:python, to the CLI options which causes ruff to treat the content as python code.

I am not familiar with the integrations other than jupyterlab-lsp, so I'm not certain whether adding this cli option to the ruff call will have unwanted effects on the other integrations, so please let me know if this is the wrong approach

Changes in this PR

  • Increased ruff requirement to 0.1.5
  • EDIT: add --extension ipynb:python to the call to ruff
  • Added extension to configuration which allows mapping file extensions to languages
  • Added test to ensure python code originating from notebooks can be handled

Other approaches

  • Keep the ruff requirement at 0.1.0 and check the version number before adding --extension to the CLI call
  • Add --extension ipynb:python regardless of configuration

@dhruvmanila
Copy link

Thanks for opening this PR and taking it forward. Do you think it would be better to add it directly as a CLI argument when invoking Ruff in python-lsp-ruff? This would be done if it's a notebook file. The flag itself is hidden but providing an option here makes it public.

We might want to confirm with the maintainers if this is the correct approach as this extension is probably being used with integrations other than jupyterlab-lsp as well.

@felix-cw
Copy link
Contributor Author

I hadn't put the dots together that making it an option makes the flag public, so sorry for not realising that.

The only reason for making the CLI flag opt-in here is because I'm unfamiliar with the other integrations and use cases, so I was worried about possible unintended side effects. I don't think there will be any, since it looks like this plugin only receives plain python anyway, but can't say for sure.

I'll update this PR to remove the public option and add the CLI flag.

@jabbera
Copy link

jabbera commented Nov 14, 2023

I need this for jupyter-lsp to work as well!

@jhossbach jhossbach added this to the v2.0.0 milestone Nov 16, 2023
@jhossbach
Copy link
Member

Hey @krassowski, can you confirm that this will resolve the issues with jupyterlab-lsp? I am not sure if there are any gotchas

@jhossbach
Copy link
Member

I have tested this locally and it looks like it works, LGTM.

@jhossbach jhossbach merged commit 42095ef into python-lsp:main Nov 25, 2023
5 checks passed
@krassowski
Copy link

Thank you all! Sorry I was away last week but I did confirm now that all works great!

@dhruvmanila
Copy link

Thank you all!

@felix-cw felix-cw deleted the extension-mapping branch November 29, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants