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

add warning whenever a value with a template is replaced by a value without template #4212

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

Conversation

mboisson
Copy link
Contributor

@mboisson mboisson commented Feb 9, 2023

… replaced by a value without template

@mboisson mboisson changed the title added a test to raise an EasyBuildError if a value with a template is… added a test to output a warning if a value with a template is… Feb 9, 2023
@lexming lexming changed the title added a test to output a warning if a value with a template is… add warning whenever a value with a template is replaced by a value without template Feb 9, 2023
Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

I looked at the related issue and I wonder if we can define a more clear behaviour here. Ideally I would trigger a straight error when the template replacement is dangerous/unintended and not do anything otherwise.

Maybe we can check if templating is disabled through EasyConfig.enable_templating and in such a case trigger an error whenever __setitem__ replaces a template. If templating is enabled probably is not an issue. What do you think? Would this approach work?

@mboisson
Copy link
Contributor Author

mboisson commented Feb 9, 2023

I looked at the related issue and I wonder if we can define a more clear behaviour here. Ideally I would trigger a straight error when the template replacement is dangerous/unintended and not do anything otherwise.

Maybe we can check if templating is disabled through EasyConfig.enable_templating and in such a case trigger an error whenever __setitem__ replaces a template. If templating is enabled probably is not an issue. What do you think? Would this approach work?

Raising an error makes existing tests fail. That's why I switched to a warning. See https://github.com/easybuilders/easybuild-framework/pull/4212/checks?check_run_id=11226883289

@boegel boegel added this to the 4.x milestone Feb 15, 2023
@boegel
Copy link
Member

boegel commented Feb 15, 2023

There's various reasons why passing a "raw" string value (with unresolved template values) makes sense, and some template values (like %(pyver)s) can only be resolved until some installation step has been reached (like prepare step, where dependencies are loaded), so a warning like this would result in quite a lot of false positives, rendering it useless...

@mboisson
Copy link
Contributor Author

There's various reasons why passing a "raw" string value (with unresolved template values) makes sense, and some template values (like %(pyver)s) can only be resolved until some installation step has been reached (like prepare step, where dependencies are loaded), so a warning like this would result in quite a lot of false positives, rendering it useless...

Hum ? That warning is precisely the opposite, i.e. when a "non-raw" string value (with resolved template values) is used to overwrite a raw one.

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.

None yet

4 participants