-
Notifications
You must be signed in to change notification settings - Fork 33
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
[FEATURE] Cleanup import statements #870
Comments
Hi @mgovers, I also have a couple of technical questions:
Anyways, I don't think that changing the expositions of the symbols in the examples above from public to private would directly simplify the package diagram. The reason why I ask is just to have a basis to start some more deep reasoning. |
Hi @emabre , Thank you for contacting us again! This definitely is still available. There's no time pressure on this, so feel free to pick this up. Can you assign yourself to this issue? On the technical questions:
Oh that's not good. They should indeed be listed in the API reference and should definitely remain public. Can you please try to make an exhaustive list of things that are publicly available that are missing from documentation? We can then go over it on a case-by-case basis. You can do this in a separate PR to isolate the changes. Good find!
In short, I believe that only things that are either input types or return types to public functions should remain public. The rest can become private. I think we can do another PR like #787 if needed (see also this announcement).
Changing exposition in itself doesn't necessarily improve the dependency graph, but it is an enabler for it. There are 2 ways you can go about this:
Cleaning the graph would e.g. amount to moving functions and class declarations to other modules and create another module that acts as an exposition. The import statement can then change to the exposition module. E.g. the following change can happen naturally (where
Most of all, we just want to get rid of the links back from Hope this helps. Martijn |
Hi @mgovers , thanks a lot for your explanation. |
Apologies, I didn't realize that that requires additional permissions. Done. Enjoy! |
The module graph for the Python wrapper is fairly convoluted (see https://power-grid-model.readthedocs.io/en/stable/advanced_documentation/python-wrapper-design.html). This is a direct consequence of the fact that many
Background
Definitions
We make the following distinctions:
.py
files (or, more correctly, anything that can be importedfrom
)import
from a module)_
)__
; double underscore)In the PGM, 'protected' and 'private' are used interchangably, as we don't really have actual symbols marked strictly private with double underscore.
Conventions on definitions
In addition, we have the following conventions:
Note: in Python, nothing is truly private and anything can technically be publicly available if you really want it. However, there is an intentionality argument that says that we can treat protected and private symbols as such. Breaking with the intentionality of the functionality is danger mode.
Conventions on changes
_core
module and/or make it private at least until it's stableConventions in code
To make sure we maintain the intentionality across the code, we do the following tricks in the code base:
TODO
Try to find a way to make the module graph less convoluted by changing expositions. Make sure to follow the above conventions.
Feel free to create new private modules and private functions where necessary.
Relates to #869 .
The text was updated successfully, but these errors were encountered: