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

chore: fix incorrect comparison operators #195

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wimlewis-amazon
Copy link

Description

Changes comparisons to use identity-comparison (is) for the singleton None, and value-comparison (==) for strings.

Why is the change necessary?

Python strings do not guarantee object identity for equal strings, so comparing self.type against a string using is might not have the intended effect. (In practice, since both values are short constant string literals from the same source file, it'll work … but it's bad practice, and generates both runtime warnings and linter warnings.)

Using is not None instead of != None is less important, but it's accepted Python style to use is/is not for the None singleton, and linters throw warnings on this as well.

Solution

How was this change tested?

tox tests/unit has the same number of failures after this change as before.


Pull Request Checklist

Please check all boxes (including N/A items)

Testing

  • Unit tests added (no behavior change)
  • Integration test added (not needed)
  • Manual testing (not needed)

Documentation

  • docs: No relevant docs
  • docstrings: No changes to public API

Title and description

  • Change type: Title is prefixed with change type: and follows conventional commits
  • References: Indicate issues fixed via: Fixes #xxx (no issue created for this PR)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

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.

1 participant