-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add reader for Trivista .tvf format #27
Conversation
Codecov ReportBase: 83.52% // Head: 84.71% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
+ Coverage 83.52% 84.71% +1.18%
==========================================
Files 49 58 +9
Lines 8390 8787 +397
Branches 1893 1952 +59
==========================================
+ Hits 7008 7444 +436
+ Misses 909 881 -28
+ Partials 473 462 -11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Note that we are changing the API slightly to expose the The documentation is adapted in #62 Would be great if you could adapt the PR sometime along the way (if anything is unclear, please leave feedback for #62). |
@LMSC-NTappy would you find some time to review another file reader? |
Absolutely! If you don't mind I will do it this weekend :) |
# You should have received a copy of the GNU General Public License | ||
# along with RosettaSciIO. If not, see <https://www.gnu.org/licenses/#GPL>. | ||
|
||
import xml.etree.ElementTree as ET |
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.
Do we care about XML vulnerabilities? I don't know much about them but maybe adding a note somewhere about this?
https://docs.python.org/3/library/xml.html#xml-vulnerabilities
(Well, just realized that many other readers make use of this module too...)
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.
Great Job! Always nice to see new file formats being supported.
Looks pretty good to me, except that I don't like all the instance variables that get initialized randomly in the TrivistaTVFReader
functions.
I suggest you move all those in the init() for clarity.
Cheers
Nicolas
Thanks for the Review @LMSC-NTappy. I agree that declaring attributes in Let me know what you think of this solution. |
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.
Let me know what you think of this solution.
Definitely better! Moving everything in the constructor is definitely going some steps further than asked but I don't see it as a problem :)
Someone maintaining this code 5-10 years from now will be thankful for that.
Description of the change
Add support for reading the .tvf-format from trivista.
Progress of the PR
upcoming_changes
folder (seeupcoming_changes/README.rst
),readthedocs
doc build of this PR (link in github checks)Minimal example of the bug fix or the new feature