don't blindly overwrite an existing easyconfig in tweak_one#2504
don't blindly overwrite an existing easyconfig in tweak_one#2504migueldiascosta merged 7 commits intoeasybuilders:developfrom
Conversation
|
What about the non-symetric handling of It also doesn't help that |
|
@Flamefire The correct implementation is what is intended: if This is clearly mentioned in docstring for I'll clarify this by better documenting the arguments of Thanks a lot for pointing this out, I appreciate thoughtful feedback like this a lot! |
…nt all arguments (more clearly)
|
Much better. However at line 140 there is already a description of what happens if the filename is None. Maybe leave that paragraph as it was and just add the mentioning of target_dir to the paragraph at 140. Although it is clear now I don't fully understand why it is written to the current working directory. One might not even have write-access to it (e.g. clusters). Writing it to a temporary directory makes more sense IMO. |
| from easybuild.framework.easyconfig.parser import EasyConfigParser | ||
| from easybuild.framework.easyconfig.tweak import find_matching_easyconfigs, obtain_ec_for, pick_version, tweak_one | ||
| from easybuild.tools.build_log import EasyBuildError | ||
| from easybuild.tools.filetools import remove_file, write_file |
There was a problem hiding this comment.
'easybuild.tools.filetools.remove_file' imported but unused
| from easybuild.framework.easyconfig.parser import EasyConfigParser | ||
| from easybuild.framework.easyconfig.tweak import find_matching_easyconfigs, obtain_ec_for, pick_version, tweak_one | ||
| from easybuild.tools.build_log import EasyBuildError | ||
| from easybuild.tools.filetools import remove_file, write_file |
There was a problem hiding this comment.
'easybuild.tools.filetools.remove_file' imported but unused
|
@Flamefire Thanks for the suggestion, I updated the docstring for W.r.t. writing to the current directory: that's a design choice I guess, and it still makes sense to me... You usually run |
|
I find the following easier to understand: And: Wrt current dir: Ok I kinda understand that in the context of However if this logic is mangled with the |
fix for bug reported by @Flamefire in #2503