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

Pylint part 2 #2339

Merged
merged 6 commits into from
Sep 1, 2022
Merged

Pylint part 2 #2339

merged 6 commits into from
Sep 1, 2022

Conversation

glennmatthews
Copy link
Contributor

Towards: #2292

What's Changed

Trying to keep these PRs a bit more manageable. :-) This one enables a few more of the highest-severity ("Error") pylint checkers and addresses them as described:

  • Enable E0203 access-member-before-definition and fix the 1 case it flagged (a minor if legitimate bug with a missing default value in UpdateObjectViewTestCase)
  • Enable E1136 unsubscriptable-object, no reports for this warning (probably got fixed along the way in the time since I initially set up the pylint configs)
  • Enable E1120 no-value-for-parameter; it reported a few warnings in test code due to Pylint not innately understanding how unittest.mock.patch works; fixed this by adding signature-mutators to the Pylint configuration.
  • Enable E1133 not-an-iterable; 5 warnings reported, all of which appear to be false positives where we have an optional parameter defaulting to None and pylint not recognizing that we're guarding with a if parameter is not None before trying to iterate over that parameter. I added type hints to these in hopes that it would get pylint to take the hint, but it appears not, so I added explicit pylint: disable for each of these as well.
  • Enhance the pylint invoke task to permit targeted execution like invoke pylint --target nautobot.utilities.testing, invoke pylint --target examples/example_plugin --recursive.

TODO

  • Explanation of Change(s)
  • n/a Attached Screenshots, Payload Example
  • n/a Unit, Integration Tests
  • n/a Documentation Updates (when adding/changing features)
  • n/a Example Plugin Updates (when adding/changing features)
  • n/a Outline Remaining Work, Constraints from Design

@glennmatthews glennmatthews self-assigned this Aug 30, 2022
@bryanculver bryanculver self-requested a review August 31, 2022 15:07
Copy link
Member

@bryanculver bryanculver left a comment

Choose a reason for hiding this comment

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

Curious of the value of keeping the type hints in here if ultimately didn't solve the lint issue.

Maybe we should do __iter__ checks instead of is not None?

Since Python doesn't do strict type enforcement at runtime maybe pylint is being overly pessimistic and wanting to see something like

if hasattr(self.relationships, '__iter__'):

@glennmatthews
Copy link
Contributor Author

Curious of the value of keeping the type hints in here if ultimately didn't solve the lint issue.

I mean in the long run I would love to turn on mypy for type checking as well... :-)

Maybe we should do __iter__ checks instead of is not None?

Since Python doesn't do strict type enforcement at runtime maybe pylint is being overly pessimistic and wanting to see something like

if hasattr(self.relationships, '__iter__'):

I'll give it a try.

@bryanculver
Copy link
Member

Curious of the value of keeping the type hints in here if ultimately didn't solve the lint issue.

I mean in the long run I would love to turn on mypy for type checking as well... :-)

yes please

@glennmatthews
Copy link
Contributor Author

I tried if hasattr(..., '__iter__') and even if isinstance(..., (list, tuple)) and neither made Pylint happy as a guard around some of these not-an-iterable cases. Explicitly casting to list (for item in list(variable)) did but I am not a big fan of that approach. I was able to address a couple of the cases by tweaking the actual logic flow a bit though.

Copy link
Contributor

@HanlinMiao HanlinMiao left a comment

Choose a reason for hiding this comment

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

LGTM!

@glennmatthews glennmatthews merged commit 8cc236d into develop Sep 1, 2022
@glennmatthews glennmatthews deleted the gfm-pylint-part-2 branch September 1, 2022 14:54
glennmatthews added a commit that referenced this pull request Sep 1, 2022
@bryanculver bryanculver mentioned this pull request Sep 9, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants