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

rename unclear *run* methods to *install_extension* + rename install_extensions to install_all_extensions #4400

Merged
merged 29 commits into from Mar 11, 2024

Conversation

lexming
Copy link
Contributor

@lexming lexming commented Dec 12, 2023

fixes #4113

  • run() -> install_extension()
  • prerun() -> pre_install_extension()
  • postrun() -> post_install_extension()
  • run_async() -> install_extension_async()
  • install_extensions() -> install_all_extensions()

@lexming lexming added the EasyBuild-5.0 EasyBuild 5.0 label Dec 12, 2023
@lexming
Copy link
Contributor Author

lexming commented Dec 13, 2023

@boegel I made your suggested changes

@boegel boegel added this to the 5.0 milestone Dec 20, 2023
@boegel boegel added the change label Dec 20, 2023
@boegel boegel changed the title rename unclear run methods to install extensions rename unclear run methods to install extensions Dec 20, 2023
easybuild/framework/extension.py Outdated Show resolved Hide resolved
@boegel
Copy link
Member

boegel commented Jan 3, 2024

@lexming We need a companion PR in easyblocks too?

@boegel boegel changed the title rename unclear run methods to install extensions rename unclear *run* methods to *install_extension* + rename install_extensions to install_all_extensions Jan 3, 2024
@boegelbot

This comment was marked as outdated.

@lexming
Copy link
Contributor Author

lexming commented Jan 26, 2024

@boegel all tests positive, this is ready

@boegelbot

This comment was marked as outdated.

@boegelbot

This comment was marked as outdated.

@lexming
Copy link
Contributor Author

lexming commented Feb 19, 2024

@boegel added unit tests for the new method Extension.install_extension_substep. I had to add various new easyblocks to the test sandbox to be able to test the case where some easyblock inherits custom or deprecated run methods from a parent class. So there are collateral changes due to those additions.

If you think that adding those extra easyblocks is overkill, we can also just test the case where an easyblock directly defines custom or deprecated run methods. For such cases, the DummyExtension easyblock is sufficient.

Regarding the use of __qualname__, the qualified name is indeed also printed as part of the string representation of the method. However, that requires parsing something like <bound method XXXX.xxx of XXX object at 0x000000000000>, which IMO is more prone to error than handling __qualname__. So I left that part unchanged.

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 merged commit 6489f96 into easybuilders:5.0.x Mar 11, 2024
63 checks passed
@lexming lexming deleted the extension-run branch March 11, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants