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 resolve_template method to EasyConfig class #4677

Merged

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Oct 10, 2024

(created using eb --new-pr)

Not only does this reduce the (non-trivial) code duplication, it also allows e.g. easyblocks to resolve a template without setting an EC parameter. e.g. self.cfg.resolve_template('lib/%(pyshortver)s') might be useful.

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@Flamefire Can you look into adding a dedicated test for the resolve_template method?

@boegel boegel added this to the release after 4.9.4 milestone Oct 12, 2024
@Flamefire
Copy link
Contributor Author

@Flamefire Can you look into adding a dedicated test for the resolve_template method?

I though it was not necessary: The tests for checking the template values already cover this via the getters and a test if ec.resolve_template(x) == resolve_template(x, ec.template_values) doesn't seem to provide much value as that is basically the implementation of the method.

So only thing which I'd say is worth checking, is whether the template dict gets initialized first time this gets called to detect easier if it gets removed by some later change, although the other tests would fail already.
But I'm not sure how to "legally" create an easyconfig instance without the template_values being initialized.

The dict gets initialized by `parse` during `__init__` and hence cannot
be empty or unset.
@Flamefire
Copy link
Contributor Author

I added a test and tried to trigger the case where template_values is not set. Turns out this is impossible, so I removed the condition.

@boegel boegel changed the title Add resolve_template function to easyconfig instances Add resolve_template method to EasyConfig class Dec 11, 2024
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel enabled auto-merge December 11, 2024 07:50
@boegel boegel merged commit ae40c24 into easybuilders:develop Dec 11, 2024
37 checks passed
@Flamefire Flamefire deleted the 20241010180706_new_pr_iTvtXUcswP branch December 11, 2024 08:57
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.

2 participants