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

Revert "Extended consider-using-tuple check to cover 'in' comparisons (#4768)" #4832

Merged
merged 2 commits into from Aug 12, 2021

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Aug 12, 2021

Description

Revert changes made in #4768 after discussion #4776.

@cdce8p
Copy link
Member Author

cdce8p commented Aug 12, 2021

@Pierre-Sassoulas I would like to revert the commit before starting to work on a replacement.
After that we might what to consider if the new "generic" use-sequence-for-iterating and use-set-for-membership checks should be enabled by default as refactoring checks. Technically they do provide a performance benefit.

The alternative would be to keep them in the code_style extension.

@cdce8p cdce8p added this to the 2.10.0 milestone Aug 12, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1124969554

  • 9 of 9 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 92.626%

Totals Coverage Status
Change from base Build 1124806342: -0.003%
Covered Lines: 13278
Relevant Lines: 14335

💛 - Coveralls

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Couldn't we replace consider-using-tuple to use-sets-for-membership for every example in this MR without reverting first ? Thiw way we would not have to think about it because the initial MR was already well thought. I guess you know better how you want to do it so I'm approving anyway :)

The alternative would be to keep them in the code_style extension.

We could keep them separated in their own checker so the decision is easier to make. I think if we want to be able to do #3512 comfortably we'll have to separate more anyway, what do you think ?

@cdce8p
Copy link
Member Author

cdce8p commented Aug 12, 2021

Couldn't we replace consider-using-tuple to use-sets-for-membership for every example in this MR without reverting first ? This way we would not have to think about it because the initial MR was already well thought.

I tried that initially, but it became such a mess that I didn't really wanted to work on it. Starting from scratch is the better and cleaner approach IMO. I'll use most of it for the new check though.

The alternative would be to keep them in the code_style extension.

We could keep them separated in their own checker so the decision is easier to make. I think if we want to be able to do #3512 comfortably we'll have to separate more anyway, what do you think ?

I thought the additional effort wouldn't be worth it. We can still separate individual checks from the rest by creating methods for them. What it comes down to IMO is if we want to enable these by default or have them in an extension.

  • If enabled by default, the RecommendationChecker might be a pretty good place (my favorite)
  • Otherwise, I would suggest the CodeStyle extension.

If you do have another idea, especially a name for the new (?) checker, I would be open to that too.

@cdce8p cdce8p merged commit fd218e2 into pylint-dev:main Aug 12, 2021
@cdce8p cdce8p deleted the revert-cs-tuple branch August 12, 2021 19:43
@Pierre-Sassoulas
Copy link
Member

Finding name is hard 😄 AppropriateContainerChecker ? Using RecommendationChecker works too. I'm convinced by Technically they do provide a performance benefit. - albeit a small one, so I'm slightly in favor of making it default without strong opinion about it.

@cdce8p
Copy link
Member Author

cdce8p commented Aug 12, 2021

Performance benefit and good coding practice.
I'll work on a PR with it as part of the RecommendationChecker for now. Can still change it later if we decide to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants