Skip to content

Linter tweaks. #115

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rpgoldman
Copy link
Contributor

@rpgoldman rpgoldman commented May 13, 2025

I'm not sure if these will be interesting to you. I was reading over the Popper code in preparation for some extensions, and fixed some issues raised by pylint and the IDE. These should have no effect on Popper's performance, but might make it easier to review and extend.

Update: I believe that the fixes in the second commit are actually important and would fix incorrect behavior under some circumstances. So perhaps this is more worthwhile than I at first thought.

Code formatting issues from the linter.

Pruned imports per linter

Provide more information in error-handler.

Replace error print statements with logging messages at error level.

Tweak error handling.

Code formatting issues from the linter.

Pruned imports per linter

Provide more information in error-handler.

Replace error print statements with logging messages at error level.

Tweak error handling.
@rpgoldman
Copy link
Contributor Author

The second commit contains a couple of corrections that might actually be important. I fixed two linter errors that I think could cause actual incorrectness:

  1. Comparison against None using equality instead of is and
  2. Mutable function default values.

rpgoldman added 2 commits May 13, 2025 14:11
Removed mutable default values.
Removed equality comparisons against None.
Looking at the README in the PyCharm IDE, discovered that it incorrectly
referenced `print_prog_score`.  This is not a function, but a method on
`Settings`.
@andrewcropper
Copy link
Collaborator

Thanks @rpgoldman ! I will review them all early next week and will hopefully merge afterwards. Thanks again.

@rpgoldman
Copy link
Contributor Author

I hope these are helpful. My IDE (PyCharm) pushes these out while I'm touring the code to figure out the innards. Mostly they are stylistic or benign, but I have found a small number of errors that are real (all software is fallible...)

@andrewcropper
Copy link
Collaborator

I get this error when trying to run the code:

NameError: name 'Optional' is not defined

Do you not have this error?

@rpgoldman
Copy link
Contributor Author

My apologies. Made a mistake in the push. Will push an update that fixes this, and also prunes some print statements that I added and that crept into the branch.

@rpgoldman
Copy link
Contributor Author

Had to force-push, sorry.

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.

2 participants