-
Notifications
You must be signed in to change notification settings - Fork 219
alphabetically sort functions and methods in module_generator module #2132
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
…t methods defined and then undefined)
easybuild/tools/module_generator.py
Outdated
|
|
||
| def __init__(self, *args, **kwargs): | ||
| """ModuleGeneratorTcl constructor.""" | ||
| super(ModuleGeneratorTcl, self).__init__(*args, **kwargs) |
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.
@damianam this is useless, if there's nothing to do in the constructor, then don't override it
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.
Well, at least now it is consistently useless. Before one of the classes had an useless constructor and one didn't. I included it in both because I thought that maybe you had an useless constructor there for some obscure reason I didn't know about...
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.
No, there is no reason, so better clean up the empty constructor in ModuleGeneratorLua instead (and drop this one here).
|
@damianam It's hard to tell from the diff, but this is just literally moving stuff around without any changes to the code in the methods, right? One thing I'm slightly worried about is that this is going to clash with #2110 which adds a I expect that resolving the conflict here rather than in #2110 is going to be easier, so I'll get #2110 merged first, and then help out if needed with resolving the conflict in here (if needed). |
|
Merging #2110 seems reasonable, but that means that this PR and the one that was coming after this one will have to wait do don't do things multiple times. Which is fine for me TBH, just a bit of pain to do things twice. |
|
@damianam I hope we can get #2110 merged soon, it's up to @victorusu now to tackle the last remaining remarks there |
sync with develop
|
Did some checks to confirm that this is just shuffling things around without making changes, tests confirm, so going in, thanks for the effort @damianam! |
Just to have a cleaner file for another PR on top