Skip to content

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented May 11, 2022

Type of Changes

Type
βœ“ πŸ”¨ Refactoring
βœ“ πŸ“œ Docs

Description

I realized during implementation that simplifiable if expression became worst and that I disagree with #1984 basically. If there are two values but one if the opposite of the other we can pretty much simplify the if expression. It look like the example from the issue come directly from this stackoverflow question but the top answer make a compelling point that this is simplifiable.

Refs #5953
Closes #5882

@coveralls
Copy link

coveralls commented May 11, 2022

Pull Request Test Coverage Report for Build 2403335689

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.51%

Totals Coverage Status
Change from base Build 2403218426: 0.0%
Covered Lines: 16230
Relevant Lines: 16993

πŸ’› - Coveralls

FLYING_THINGS = ["bird", "plane", "superman", "this example"]

def is_flying_animal(an_object):
if isinstance(an_object, Animal) and an_object in FLYING_THINGS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to expect a message here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's not raising anything right now, so I did not expect it to work I think it's a problem and that's why this issue is labelled discussion:

I realized during implementation that simplifiable if expression became worst and that I disagree with #1984 basically. If there are two values but one if the opposite of the other we can pretty much simplify the if expression. It look like the example from the issue come directly from this stackoverflow question but the top answer make a compelling point that this is simplifiable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah okay.

I'm not sure whether I think we should raise here. I'll let others give their opinion as well.

Copy link
Member

@jacobtylerwalls jacobtylerwalls May 26, 2022

Choose a reason for hiding this comment

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

I think #1984 was correct, because even if the two values are given opposing boolean values, we can't simplify, because they might have had state before.

E.g. this raises TypeError because b had state:

def x():
  a = []
  b = []
  if bool('special test'):
    a = True
  else:
    b = False
  return a + b

But if we refactor to return bool('special test'), then the behavior is different.

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas May 29, 2022

Choose a reason for hiding this comment

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

You're right, thank you for your input πŸ‘ I think this is still simplifiable because clearly the code style is horrible (with type redefinition of a and b and the conditional can still be simplified : clearly we don't have this type of construct in the primer πŸ˜„) but it's another MR's scope.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs review πŸ” Needs to be reviewed by one or multiple more persons label May 11, 2022
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the simplifiable-if-expression branch from 3ef2922 to 4e1c4f2 Compare May 12, 2022 09:19
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the simplifiable-if-expression branch 2 times, most recently from c9a78b8 to 9425897 Compare May 29, 2022 05:13
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the simplifiable-if-expression branch from 9425897 to 83c3ad3 Compare May 29, 2022 05:36
@Pierre-Sassoulas Pierre-Sassoulas removed Discussion πŸ€” Needs review πŸ” Needs to be reviewed by one or multiple more persons labels May 29, 2022
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the simplifiable-if-expression branch from 83c3ad3 to 87d3988 Compare May 29, 2022 05:38
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the simplifiable-if-expression branch from 87d3988 to 35fe42b Compare May 29, 2022 05:47
@github-actions
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.15.0 milestone May 29, 2022
@Pierre-Sassoulas Pierre-Sassoulas merged commit 6e69273 into pylint-dev:main May 29, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the simplifiable-if-expression branch May 29, 2022 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The simplifiable-if-expression message description should not only mention bool(test)
4 participants