Skip to content
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

Make private functions and classes more explicit #2427

Open
valentinsulzer opened this issue Nov 3, 2022 · 5 comments
Open

Make private functions and classes more explicit #2427

valentinsulzer opened this issue Nov 3, 2022 · 5 comments
Labels
difficulty: hard Will take several weeks

Comments

@valentinsulzer
Copy link
Member

Make it clearer which methods should not be accessed by users, by adding an underscore at the start of their name

@valentinsulzer valentinsulzer added the difficulty: hard Will take several weeks label Nov 20, 2022
@valentinsulzer valentinsulzer mentioned this issue Dec 20, 2022
8 tasks
@Saransh-cpp
Copy link
Member

We should also look into adding __all__ to pybamm modules

Copied from the PR linked above

@agriyakhetarpal
Copy link
Member

There was an attempt by @arjxn-py on adding __all__ with the mkinit CLI-based package to speed up the PyBaMM import, but there were problems with that approach (#3732) because it would be messy to add it to several files at once and generate it everytime. Here is an older thread I found, probably not relevant anymore: #2436

It would make sense to start with smaller modules like util.py, doc_utils.py, install_odes.py (the latter should just have one method of usage – through the entry point). Also, version.py should be a private module too (pybamm._version.__version__ instead of pybamm.version.__version__), and it could be altered in favour of using importlib.metadata.version() to fetch the version from declarative metadata – in 2024 we should not need to import a package to obtain its version

@arjxn-py
Copy link
Member

arjxn-py commented Mar 7, 2024

It should be possible to add __all__ only as well with the help of mkinit, how'd we want to have it though -

  • have __all__ added to all the submodules? (Not Recommended - as it would again be messy with several file changes)
  • have a recursive __all__ added to the root __init__.py (Recommended - See this commit)
  • have a non-recursive __all__ added to the root __init__.py (See this commit, Makes lesser sense as it only includes direct attributes of pybamm)

@agriyakhetarpal
Copy link
Member

The second one makes the most sense to me, it should decide the namespace when we import PyBaMM.

The third one would need a bit more refining because we have things like update_LD_LIBRARY_PATH which should not be exposed

@arjxn-py
Copy link
Member

arjxn-py commented Mar 8, 2024

The third one would need a bit more refining because we have things like update_LD_LIBRARY_PATH which should not be exposed

We also have these attributes in the second too, as ultimately:

  • First is the superset of Second & Third &
  • Second is the superset of Third

Currently Protected modules are exposed, but their attributes are not & Private modules and their attributes are not exposed.

I shall open a PR going forward with the second at the moment but to refine this i'd welcome reviews to omit attributes based upon different preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: hard Will take several weeks
Projects
None yet
Development

No branches or pull requests

4 participants