Skip to content
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

Support for lowercase module naming scheme #3199

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

lucamar
Copy link

@lucamar lucamar commented Feb 11, 2020

As discussed already by @gppezzi with @boegel, I'm submitting a pull request to support the lowercase module naming scheme in EasyBuild (release 4).

@lucamar lucamar changed the title Support for lower case module naming scheme Support for lowercase module naming scheme Feb 11, 2020
@smoors
Copy link
Contributor

smoors commented Feb 17, 2020

@lucamar we probably need a test for this new MNS, see test/framework/module_generator.py.
are you up for that?

from easybuild.tools.module_naming_scheme.utilities import det_full_ec_version


class LowercaseModuleNamingScheme(ModuleNamingScheme):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucamar Wouldn't it make sense to derive this from EasyBuildMNS, and rename it to LowercaseEasyBuildMNS?

Copy link
Author

@lucamar lucamar Apr 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boegel Sure, I can change it to a subclass of EasyBuildMNS and the rename the file as lowercase_mns.py: I think that initially we just copied the structure of categorized_mns.py as a template

:param ec: dict-like object with easyconfig parameter values (e.g. 'name', 'version', etc.)
:return: string with full module name <name>/<installversion>, e.g.: 'gzip/1.5-goolf-1.4.10'
"""
return os.path.join(ec['name'], det_full_ec_version(ec)).lower()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified to

return super(EB_LowercaseEasyBuildMNS, self).det_full_module_name().lower()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint, however if I change it as you suggested, I keep getting the NameError below:

NameError: global name 'EB_LowercaseEasyBuildMNS' is not defined

Maybe I'm missing something trivial...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucamar that's because you did not yet implement the suggested change by @boegel to LowercaseEasyBuildMNS (see above).

@lucamar
Copy link
Author

lucamar commented Apr 21, 2020

@lucamar we probably need a test for this new MNS, see test/framework/module_generator.py.
are you up for that?

Thanks for your feedback @smoors and sorry for my late reply. Should I add a test for the lowercase module naming scheme defining something similar to test_mns() in module_generator.py?

@smoors
Copy link
Contributor

smoors commented Apr 26, 2020

@lucamar the test should probably be on the same level as test_module_naming_scheme(), which is the test for the default EasyBuildMNS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants