-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Add support for TMod version 3.1 #935
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
Conversation
|
Hello @victorusu, Thank you for updating! Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide Comment last updated at 2019-11-25 21:41:24 UTC |
vkarak
left a comment
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.
It seems that Tmod 3.1 has a completely different output and its basic command is also diverse. I think it would make sense to define a separate modules implementation, named tmod31. For symmetry, we can define tmod32 and make tmod and alias to that.
…ature/modules3_1
Address PR remark to split TMod into two classes Add 'tmod31' as possible supported module
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 think that everything in all four implementations (TMod, TMod31, TMod4 and LMod) boils down to three basic differences (correct me if I'm wrong):
modulecmdcommand (python bindings are got in the same way as soon as you have the modulecmd)- Command to get the version string (version strings are all in . format at least)
- How python bindings are executed.
Thus if you abstract only these three things in the base class, which imho, could simply be the standard TModImpl, then the subclasses would have just to override these three functions only. Currently, there is lots of unnecessary code duplication between all the three modules systems.
Codecov Report
@@ Coverage Diff @@
## master #935 +/- ##
=========================================
- Coverage 92% 91.7% -0.31%
=========================================
Files 81 81
Lines 11030 11076 +46
=========================================
+ Hits 10148 10157 +9
- Misses 882 919 +37
Continue to review full report at Codecov.
|
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 will merge this as soon as you update the documentation. It's not exactly what I want, because the code duplication is not addressed, but I will merge it since it has been standing for long time. I will open a follow up issue on refactoring the modules backend implementation. See #1063.
Extend the TMod implementation to add support for modules 3.1.
Fixes #784.