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

LGTM: various fixes for using string operator instead of numeric #2649

Merged
merged 1 commit into from Oct 25, 2019

Conversation

luzpaz
Copy link
Contributor

@luzpaz luzpaz commented Oct 22, 2019

No description provided.

@vocx-fc
Copy link
Contributor

vocx-fc commented Oct 24, 2019

These changes test for None. It is fine to substitute the equality comparison == for is, however, it is a bit more Pythonic to simply test for the existence of the object. If the object is None, then it is equal to not object.

if something == None:
    proceed()

if something is None:  # better
    proceed()

if not something:  # best
    proceed()

Using object and not object tests for existence and non-existence.

if something:  # exists
    proceed()
 
if not something:  # does not exist
    proceed()

@yorikvanhavre
Copy link
Member

For the story, the rationale I read is that something is None is faster than something == None as it compares pointers and not values

@yorikvanhavre yorikvanhavre merged commit 1bc3302 into FreeCAD:master Oct 25, 2019
@luzpaz luzpaz deleted the lgtm-string-operator-swap branch October 25, 2019 14:25
@joelgraff
Copy link
Contributor

I can't help but comment on the is None vs not issue. I'm sure we're all familiar with it - that not is a test to see if something is "falsey". Thus, None, 0, False, [], {}, and '' all qualify.

The problem for me is that not is testing for two very different conditions and returning the same result. That is, 'False', 0, [], {} and '' are all about state. None is about existence. Failing to distinguish whether not something means it's is just empty / zero / false or means it doesn't exist at all usually ends up biting me.

Thus, I expressly avoid using not if I'm really testing for None. At very least, I test for it separately.

@vocx-fc
Copy link
Contributor

vocx-fc commented Nov 4, 2019

Joel, I think in all cases mentioned above the test is for existence, which is why I think using not something is fine. There is no separate test for empty content (empty value). If this exists, proceed, if it doesn't exist, then don't proceed.

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

4 participants