-
Notifications
You must be signed in to change notification settings - Fork 14
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
Clearly define public vs. private interface #41
Comments
Here is a possible suggestion, adapting from yours, with the philosophy that we can make methods public easily later if we want to, while the reverse is complex for backwards incompatibility:
What do you think? Happy for different suggestions of course |
Also, for renaming, some suggestions:
|
Thanks a lot! I think I agree on the public / private suggestions (at least, I didn't see anything that I didn't agree with at first glance).
Yeah, that makes sense. Is output-parsing also within the scope of this library? If so, we could consider either a structure
or flat:
I tend to favor the flat structure, although that means right now the options are just
Definitely. I think this opens up a separate discussion, see #42
Yeah, |
If you would like my 2 centimes, I would prefer the nested structure, were it not for the fact that you will run into the problem when importing from |
always appreciated 👍
Ha, I hadn't even considered that. Though it would technically work because What's still bugging me here is that We could also ditch the "input" from the module name completely, and have the classes be importable as e.g. |
This makes me think that the actual "generators" are currently embedded in the |
Absolutely agree - for now we can just design the naming here to make sure it fits when we move that over.
? |
Let's go with this |
Separate the public and private interface, as detailed in issues aiidateam#41,42,25. The duplicated code from CpInputFile and PwInputFile has been moved into the common base, which was renamed and made private. The constants are now given as a SimpleNamespace, instead of defined at the top-level of their module. The reason for this is that we want to be compatible with adding version-specific constants later on. This could now be done by adding a 'get_constants' function that takes the QE version as input, and returns the SimpleNamespace of corresponding constants.
Separate the public and private interface, as detailed in issues #41,42,25. The duplicated code from CpInputFile and PwInputFile has been moved into the common base, which was renamed and made private. The constants are now given as a SimpleNamespace, instead of defined at the top-level of their module. The reason for this is that we want to be compatible with adding version-specific constants later on. This could now be done by adding a 'get_constants' function that takes the QE version as input, and returns the SimpleNamespace of corresponding constants.
Ahead of the
2.0.0
release, I think we should clearly define public vs. private interface:__all__
defined__all__
in the module where it is defined.ipython
should autocomplete only the public parts whenc.IPCompleter.limit_to__all__ = True
is set.I can do the changes to the code, but the important question is: which parts should be in the public interface? We have:
qe_tools
: publicparsers
: publiccpinputparser
: ?CpInputFile
: public, directly underparsers
pwinputparser
: ?PwInputFile
: public, directly underparsers
parse_k_points
: ?qeinputparser
: ?QeInputFile
: public, directly underparsers
get_cell_from_parameters
: ?get_structure_from_qeinput
: ?parse_atomic_positions
: ?parse_atomic_species
: ?parse_cell_parameters
: ?parse_namelists
: ?str2val
: ?strip_comment
: ?generators
: publiccell_conversion
: ?get_parameters_from_cell
: public (here, or directly undergenerators
)?utils
: ?_qe_version
: privateparse_version
: privateexceptions
: ?constants
: ?In
aiida-quantumespresso
, the following are used:qe_tools.constants
qe_tools.parsers
,CpInputFile
andPwInputFile
are used in code, their "real" location (e.g.qe_tools.parsers.cpinputparser.CpInputFile
is used in docstringsqe_tools.utils.exceptions.ParsingError
is used in testing@giovannipizzi @sphuber @lekah what's your opinion? If needed, now would also be a good time to move/rename things.
The text was updated successfully, but these errors were encountered: