-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: enabling structured package #278
Conversation
…converter-xml2py into feat/structured_package
…converter-xml2py into feat/structured_package
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.
Several comments.
- Avoid generate the content at the same time you write the file, so you can test the content without writing the file.
- You do not need to use
fid.close
if you are working inside a context manager. - Readability is important. Please group statements doing an specific function, under a function with proper name. Sometimes the code is difficult to follow if you have many many statements doing "standard" python (not grouped in functions). I mean, you either comment more your code, so we know what is doing each block of statements, or you group that block under a function with a representative name.
Co-authored-by: Kathy Pippert <[email protected]>
Co-authored-by: Kathy Pippert <[email protected]>
Thank you very much for your review @PipKat! |
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.
Just a minor remark. Other than that, I trust my past self. xD
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.
Overall LGTM but as you may imagine, I couldn't review in depth all the logic. Left some comments. Request review once solved.
Co-authored-by: Roberto Pastor Muela <[email protected]>
Type hint has been implemented in most of the package - except in the |
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.
LGTM! Thanks @clatapie for your hard work on this
This PR modifies the structure of the created package.
Structure of the package before:
Now:
This new structure is directly obtained from the XML repository.
TO DO: