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

Adapted fix to work identical to format #10999

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Philipp-Thiel
Copy link
Contributor

Summary

The fix for E203 now produces the same result as ruff format in cases where a slice ends on a colon and the closing square bracket is on the following line.

Refers to #10973

Test Plan

The minimal reproduction case in the ticket was added as test case producing no error. Additional cases with multiple spaces or a tab before the colon where added to make sure that the rule still finds these.

Copy link

github-actions bot commented Apr 17, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -3 violations, +0 -0 fixes in 2 projects; 42 projects unchanged)

RasaHQ/rasa (+0 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- rasa/shared/core/generator.py:758:60: E203 [*] Whitespace before ':'
- tests/nlu/featurizers/test_lm_featurizer.py:699:41: E203 [*] Whitespace before ':'

mlflow/mlflow (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- tests/tracking/test_rest_tracking.py:72:27: E203 [*] Whitespace before ':'

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
E203 3 0 3 0 0

@MichaReiser MichaReiser added bug Something isn't working rule Implementing or modifying a lint rule labels Apr 17, 2024
@zanieb
Copy link
Member

zanieb commented Apr 17, 2024

Are these ecosystem checks showing new false negatives?

Copy link
Member

@MichaReiser MichaReiser 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 left an understand question and I think it might be good to add two more tests but this looks good to me.

Comment on lines +89 to +92
#: E203 tab before :
predictions = predictions[
len(past_covariates) // datamodule.hparams["downsample"] :
]
Copy link
Member

Choose a reason for hiding this comment

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

Can we add two more examples where the line end:

  • in a comment
  • in a line continuation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, those would both be legal continuations. Didn't think about that, but will try to add those in the coming days

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the comment case, but can't figure out how to do the line continuation, without basically rewriting the rule since the \ doesn't show up in the token stream. I could try to adapt E502 for it, if you consider the case important.

On the bright side, both ruff format and black remove a \ in the example situation anyway so there would be no continuing conflict.

Copy link
Member

Choose a reason for hiding this comment

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

You could try to use SimpleTokenizer when you found a potential violation to lex the text coming right after the token and see if the next token is a line continuation.

@Philipp-Thiel
Copy link
Contributor Author

Are these ecosystem checks showing new false negatives?

I would say yes. They all have the same structure as the minimal reproducible example in the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants