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

E302 and E305 cause the linter and formatter to make conflicting changes in notebooks #10228

Closed
vancromy opened this issue Mar 4, 2024 · 9 comments · Fixed by #10291
Closed
Assignees
Labels
notebook Related to (Jupyter) notebooks rule Implementing or modifying a lint rule

Comments

@vancromy
Copy link

vancromy commented Mar 4, 2024

The new rules introduced in ruff v0.2.2 via #9266 are causing the ruff linter to make conflicting changes in notebook cells that the formatter is then trying to fix.

Here is an example notebook which reproduces the bug:
image

ruff config:

[tool.ruff]
extend-include = ["*.ipynb"]

[tool.ruff.lint]
select = [
    "E",
]

If you run the commands in this succession and look at the diffs you will notice that the linter adds two lines at the end of:

  1. Cell 3 (the one just printing out the variable some_computation). This is rule E302 in action
  2. Cell 4 (after the function definition). This is rule E305 in action.

The commands I ran:

  1. ruff check --force-exclude --preview --fix test.ipynb
  2. ruff format --force-exclude --preview test.ipynb

(The --force-exclude is because I discovered this bug through pre-commit and I was trying to mimic the pre-commit behaviour)

This behaviour happens from v0.2.2 upwards (I've also tested v0.3.0) as long as the preview flag is on.

Let me know if the MRE needs more explanation.

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule notebook Related to (Jupyter) notebooks labels Mar 4, 2024
@MichaReiser
Copy link
Member

Thanks for the excellent write up. Agree, that is a bug in E302 and E305. It should not enforce blank lines at the end of a cell.

@hoel-bagard
Copy link
Contributor

I can try to fix it if no one has started working on it yet.

@9128305

This comment was marked as outdated.

@MichaReiser
Copy link
Member

@9128305 this should be fixed in the next release. See #10098

@hoel-bagard
Copy link
Contributor

@MichaReiser You are only talking about the comment above, right ?

@MichaReiser
Copy link
Member

MichaReiser commented Mar 6, 2024

@hoel-bagard Yes, my comment was specific about compatibility within typing stub file (pyi). The incompatibility documented in this issue isn't solved.

@dhruvmanila any recommendation how to fix this or is this something you could take on?

@dhruvmanila
Copy link
Member

We do have custom handling for rules in Notebooks (B018, B015), so I think this should be possible.

There's a helper function which checks if it's the last expression in the cell. We could either use the same function or provide a similar function depending on what the needs are here and check if we're at the cell boundary. If so, ignore checking for blank lines.

In the linked function, the tokenizer would start at the end of the expression. But for E302, we will need to start from the cell boundary and end at the statement start position. This will require a new function.

I'm not sure how exactly to get the range of the entire function / class as these rules are token-based and not AST-based. For an AST node we can directly get the end of the function / class node. It might be that I would need to look into it in detail to understand that.

@MichaReiser I'm not sure if I want to look into it right now unless it's a priority.

@MichaReiser
Copy link
Member

Thanks @dhruvmanila. @hoel-bagard would you be interested to look into this issue? I'm sure @dhruvmanila would reply to any questions you have

@hoel-bagard
Copy link
Contributor

@MichaReiser I would be interested to look into it yes. I'll start by looking at the references given by @dhruvmanila, and ask if I get stuck on something.

dhruvmanila pushed a commit that referenced this issue Mar 25, 2024
…cell (#10291)

## Summary

Closes #10228

The PR makes the blank lines rules keep track of the cell status when
running on a notebook, and makes the rules not trigger when the line is
the first of the cell.

## Test Plan

The example given in #10228 is added as a fixture, along with a few
tests from the main blank lines fixtures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook Related to (Jupyter) notebooks rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants