Skip to content

Adapted fix to work identical to format#10999

Merged
charliermarsh merged 5 commits intoastral-sh:mainfrom
Philipp-Thiel:E203_line_break
Jun 8, 2024
Merged

Adapted fix to work identical to format#10999
charliermarsh merged 5 commits intoastral-sh:mainfrom
Philipp-Thiel:E203_line_break

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.

@github-actions
Copy link
Contributor

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 -4 violations, +0 -0 fixes in 2 projects; 48 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 -2 violations, +0 -0 fixes)

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

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

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
E203 4 0 4 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.

@charliermarsh charliermarsh merged commit 7509a48 into astral-sh:main Jun 8, 2024
@charliermarsh
Copy link
Member

I think this looks like an improvement. The ecosystem checks are improvements, even if it's not a perfect match for the formatter (we don't check if values are complex).

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.

4 participants