-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
Adds recursive __all__
dictionary
#4040
Conversation
Also related to #2427 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4040 +/- ##
===========================================
- Coverage 99.58% 99.58% -0.01%
===========================================
Files 261 287 +26
Lines 21454 21723 +269
===========================================
+ Hits 21366 21632 +266
- Misses 88 91 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @arjxn-py – overall, I believe that adding __all__
to the codebase makes a lot of sense, it's the more modern way of bringing structure to the public API, plus providing efficient tab completions for IDEs and text editors.
I wonder if doing this in a staggering manner, i.e., across multiple pybamm/module/submodule/__init__.py
files makes more sense, in comparison to adding them in the same file? We have way too many changes in a single file right now, it would become difficult to keep track of where these symbols and attributes come from (some in-line comments could also be of help). Keeping everything split across files will be just a one-time change to review (though it would have to be thorough), and then adding to items to multiple __all__
lists will be simpler, too, since they will be smaller. I suppose you're using the mkinit
tool right now that has come up in our previous discussions – the main issue with it was that we would have needed to ask contributors to run a command every time they modified something for the API, and we don't want that additional complexity. Splitting this across multiple files should make the structure easier to follow and then manually modify on future changes, without the use of the tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @arjxn-py! I think this looks all good to me, unless there is something more that you plan to do here and push further changes.
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Planned to add some directives to contribution guide as well but I believe it should be fine without that, we just have to little bit more careful on the PRs modifying method & attribute names or adding new of them. |
Feel free to merge it after approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, @arjxn-py! I'll request another review from @Saransh-cpp, we can merge soon after.
Edit: could you fix the conflicts?
Is this something that needs to be updated frequently? |
No, as long as we're not changing the API frequently. It does need to be updated when adding new features, though. The goal will be that once we have a stable public-private API demarcation, |
Just a gentle ping to @Saransh-cpp to review this and go forward with it if looks fine, Thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @arjxn-py! Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The coverage failure is related to an upstream change and the patch coverage is passing so approving again, thanks, @arjxn-py!
* Add recursive `__all__` dict with every submodule currently * Adds `__all__` to submodules recursively * Suggestions from code review * Fix breaking import --------- Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Description
Been proposed in #3866
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: