-
Notifications
You must be signed in to change notification settings - Fork 88
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
Register BitRound (fix #346) #347
Conversation
Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-08-01 18:53:49 UTC |
ignoring pep8speaks in favor of keeping the file consistent, though I imagine that it could use some cleanup, e.g. alphabetical ordering of the registrations. |
Should it not now be enough to put it in the entrypoints? Or maybe there's no point doing that because we know numcodecs must be imported. |
We could certainly replace all of the imports in |
OK, well let's not for this PR. For record, an entrypoints-only (or
Certainly let's fix this! |
Do we want to include a release note? |
Just waiting on the RTD for review and then I'll release 0.10.2. |
Do we know why tests didn't catch this? |
Ignoring the Windows failure:
knowing that it's unrelated, but there may be an issue in this repo that needs fixing. |
Wonder if we should test the batteries included codecs as part of the registry tests |
All current tests specifically do A simpler fix would likely be to have a "new codec" issue with a dedicated checklist. (cc: @MSanKeys963) |
For example, yeah 👍 |
Filed issue ( #348 ) as a placeholder linking to this testing discussion. Feel free to add anything else there |
Thanks for handling this Josh! 🙏 |
#299 was missing the registration of the class itself. We likely need a test to detect all classes that weren't registered.
TODO:
tox -e py310
passes locallytox -e docs
passes locally