Skip to content

STYLE: Add ruff check to avoid multiple with statements #53436

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

Closed
DeaMariaLeon opened this issue May 29, 2023 · 10 comments
Closed

STYLE: Add ruff check to avoid multiple with statements #53436

DeaMariaLeon opened this issue May 29, 2023 · 10 comments
Assignees
Labels
Code Style Code style, linting, code_checks

Comments

@DeaMariaLeon
Copy link
Member

DeaMariaLeon commented May 29, 2023

Currently, it is possible to nest multiple consecutive context managers in the code.
It can be avoided by adding the following to ruff checks:

https://beta.ruff.rs/docs/rules/multiple-with-statements/

This should be added to .pre-commit-config.yaml

@DeaMariaLeon DeaMariaLeon added the Code Style Code style, linting, code_checks label May 29, 2023
@DeaMariaLeon DeaMariaLeon changed the title STYLE: Add ruff check to avoid multiple with statements STYLE: Add ruff check to avoid multiple with statements May 29, 2023
@snorfyang
Copy link
Contributor

Maybe in pyproject.toml?

@DeaMariaLeon
Copy link
Member Author

Thank you for telling me @snorfyang. Since I'm not sure, I'm tagging @MarcoGorelli.

@MarcoGorelli
Copy link
Member

yup, that's right, along with the other ruff codes - PRs welcome if this interests anyone! if you add the code for this check to pyproject.toml, and then can get pre-commit run ruff --all-files to pass, then you've done this correctly

@snorfyang
Copy link
Contributor

take

@phofl
Copy link
Member

phofl commented Jun 1, 2023

Maybe a stupid question, but why would we want to disallow this globally? 2 with statements might have different scope

@DeaMariaLeon
Copy link
Member Author

It would still be possible to have two or more scopes, it is just the way to write them that would change. Look at Marco's comment here if you wish.
He avoids writing with twice but he is still managing the warning and the error.

@phofl
Copy link
Member

phofl commented Jun 1, 2023

They might refer to different Code parts though, not all with statements might exactly have the same length.

@MarcoGorelli
Copy link
Member

I presume ruff has thought about this and they only recommend collapsing the 'with' statements if they have the same scope - should be fine, and it's one of the autofixable ones

@phofl
Copy link
Member

phofl commented Jun 1, 2023

I still have concerns about readability, imo we should at least escape tests from this, since this reads terrible, I highlighted one such occurrence in the pr that is linked here

@MarcoGorelli
Copy link
Member

it's a matter of preference I guess, if you don't like it then let's leave this one out, no worries, we don't need to enable all of ruff's checks 😃 let's close then, thanks all for the issue/discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants