-
Notifications
You must be signed in to change notification settings - Fork 162
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
initial property parse #1325
initial property parse #1325
Conversation
okay cleaned this up a bit. I am currently just spot checking this with the following script:
|
OK this is awesome. I will review and merge today probably then rebase the tree stuff |
sounds good. i am working on a few more but thought those could be in a separate PR. Also i just full copied the utils files from parser directory, we may be able to prune it. |
for more information, see https://pre-commit.ci
Dependency resolution is a WIP but looks fine right now. For us to remember in the future, offline discussions led to us basically having parsers with dependencies where the order that the subparsers are called is resolved by making some sort of dependency tree and making sure things are called in the right order. It's not clear to me how to handle if only some programs have dependencies (e.g. property A in gaussian requires B and C but property A in Orca requires B and D)? |
known_codes = ["gaussian"] | ||
|
||
@staticmethod | ||
def gaussian(file_handler, ccdata) -> list | None: |
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.
This is too much to go over in a code review but this reminds me of https://craftinginterpreters.com/representing-code.html#the-expression-problem: is it better for us to have properties as classes/programs as methods, or programs as classes/properties as methods? This implements the former, since the latter has been a mess, and I think doing anything fancier (visitor) is not user-facing.
My one concern here is that this method is public. Will it be able to be made private so a user never has to specialize their code for their specific program? (Visibility is a breaking API change.)
return None | ||
|
||
@staticmethod | ||
def parse(file_handler, program: str, ccdata) -> list | None: |
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.
This is the "same" in every parser. The atombasis.known_codes
can be abstracted if it's made a classmethod
, then it becomes cls.known_codes
, getattr(cls, program)
, etc., which just leaves the return type. Shouldn't the return type eventually be https://docs.python.org/3/library/typing.html#typing.Self?
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.
i do agree in abstracting this would be good. I am a bit unclear on the last sentence on how to use getattr with it and what the return type is?
I have to read a bit more about the use of typing.Self, but you're probably right. I can open an issue about it if its an eventual change if you are okay letting it through without this at the moment.
for more information, see https://pre-commit.ci
Since this is approved I am going to merge. I opened a meta issue and linked the conversation there to decide exactly how to proceed with some of the minor changes |
…tations initial property parse
Starting to add other property parsers. These aren't thoroughly tested so will mark this WIP for a bit.