Adds recursive __all__ dictionary

[arjxn-py]

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.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ 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)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ 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:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[arjxn-py]

Also related to #2427

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (c687134) to head (d87955b).
Report is 1 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

[agriyakhetarpal]

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.

[agriyakhetarpal]

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.

[arjxn-py]

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.

[arjxn-py]

Feel free to merge it after approval.

[agriyakhetarpal]

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?

[kratman]

Is this something that needs to be updated frequently?

[agriyakhetarpal]

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, __all__ will reflect the public API.

[arjxn-py]

Just a gentle ping to @Saransh-cpp to review this and go forward with it if looks fine, Thanks :)

[Saransh-cpp]

Thanks, @arjxn-py! Looks good to me!

[agriyakhetarpal]

The coverage failure is related to an upstream change and the patch coverage is passing so approving again, thanks, @arjxn-py!