Skip to content

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.

@rpgoldman
Copy link
Contributor Author

I will retest this now and verify that it is consistent with main. After that, it would be great to merge this, and it could be squash-merged, since the history isn't very interesting -- it's all tweaks to make linters happy.

@rpgoldman
Copy link
Contributor Author

I have just checked test results on this branch. All the ordinary domains replicate results on main. The noisy ones don't replicate perfectly, but looks like the changes are within the bounds of non-determinism. So I believe that this can be merged.

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