Skip to content

STYLE enable ruff PLW2901 #51797

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Mar 17, 2023
Merged

STYLE enable ruff PLW2901 #51797

merged 15 commits into from
Mar 17, 2023

Conversation

ErdiTk
Copy link
Contributor

@ErdiTk ErdiTk commented Mar 5, 2023

ErdiTk added 2 commits March 3, 2023 22:58
@ErdiTk
Removed "PLW2901" and added in main folders where needs to be enabled…
1f698b9 
… gradually.

Currently removed 10 errors generated outside the main folders.
@ErdiTk
Merge branch 'main' into stype_enable_ruff_plw2901
9cf095d 
# Conflicts:
#	doc/source/whatsnew/v2.1.0.rst
@MarcoGorelli MarcoGorelli changed the title Stype enable ruff plw2901 STYLE enable ruff PLW2901 Mar 5, 2023
MarcoGorelli
MarcoGorelli reviewed Mar 5, 2023
View reviewed changes
@mroeschke mroeschke added the Code Style Code style, linting, code_checks label Mar 6, 2023
MarcoGorelli
MarcoGorelli suggested changes Mar 6, 2023
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for working on this

to be honest, I'm no longer sold on enabling this code, having seen the changes it entails it probably makes things less clear more often than not. this is my fault for having suggested the issue, sorry about that

the line_stripped change one is nice though, that's probably an improvement

I'd suggest we:

  • keep the line_stripped change
  • revert the rest
    and then we can get this in

@ErdiTk
Copy link
Contributor Author

ErdiTk commented Mar 6, 2023

Hi @MarcoGorelli, I just pushed an update of the PR. I renamed all the variables by setting a more intuitive variable name. Please let me know if it's fine for you. In that case I will continue like this also for the other sections.

@ErdiTk
Copy link
Contributor Author

ErdiTk commented Mar 6, 2023

thanks for working on this

to be honest, I'm no longer sold on enabling this code, having seen the changes it entails it probably makes things less clear more often than not. this is my fault for having suggested the issue, sorry about that

the line_stripped change one is nice though, that's probably an improvement

I'd suggest we:

* keep the `line_stripped` change

* revert the rest
  and then we can get this in

Oh no problem, no need to say sorry. To be fair I should've thought a bit more on the variable names. Will wait for a feedback on the last push before continuing.

MarcoGorelli
MarcoGorelli suggested changes Mar 7, 2023
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for updating, this does look better!

just left a couple of comments

@MarcoGorelli
Copy link
Member

looks like most tests are failing, could you check what it might be please?

@ErdiTk
Copy link
Contributor Author

ErdiTk commented Mar 7, 2023

looks like most tests are failing, could you check what it might be please?

The line "pandas/core/*" was repeated twice in the pyproject.toml file. I didn't see it in the merge conflict. Now should be fine.

@ErdiTk
Copy link
Contributor Author

ErdiTk commented Mar 8, 2023

The error seems to be connected to my changes in the scripts/no_bool_in_generic.py file. I will investigate and fix it today.

@ErdiTk
Copy link
Contributor Author

ErdiTk commented Mar 8, 2023

Hi @MarcoGorelli, I think I completed all the requested changes. Please let me know if I missed something. Just updated the branch with main and fixed the bug I introduced.

@ErdiTk
Copy link
Contributor Author

ErdiTk commented Mar 16, 2023

@MarcoGorelli Sorry I have been away in the last few days. I just pushed the last merge with the main branch. Ideally there should be no more errors. If that is the case do you need any further update of the branch?

MarcoGorelli
MarcoGorelli approved these changes Mar 17, 2023
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Looks good to me, hopefully this rule will help save some future bugs

Merging then (apologies for the delay, was away last week)

scripts/validate_docstrings.py
position = None
continue
item = line.strip()
item = line_stripped.strip()
Copy link
Member

Choose a reason for hiding this comment

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

lol, the improved name you've chosen actually makes it clear that this line is probably unnecessary 😄 if you wanted to clean this up in a follow-up, that'd be welcome

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will defo follow up this topic. From this Sunday I will have time to dedicate to it :)

@MarcoGorelli MarcoGorelli added this to the 2.1 milestone Mar 17, 2023
@MarcoGorelli MarcoGorelli merged commit 1bdc98c into pandas-dev:main Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoGorelli MarcoGorelli MarcoGorelli approved these changes

Assignees
No one assigned
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.

STYLE enable ruff PLW2901
3 participants
@ErdiTk @MarcoGorelli @mroeschke