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

Small improvements to ModuleLoadEnvironment #4754

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

Flamefire
Copy link
Contributor

I just checked #4653 and found a few possible improvements:

  1. Using an "unknown" state and requiring callers to update that is error-prone. Comparing self.install_lib_symlink == LibSymlink.LIB64_TO_LIB might silently fail if it is forgotten. Use a property with auto-init instead
  2. Isn't def __init__(... var_type=ModEnvVarType.PATH_WITH_FILES...) clearer in what the default is instead of using and checking for None?
  3. for attr in self.__dict__: yield attr, getattr(self, attr) can be made simpler and faster by using yield from self.__dict__.items() or just returning the latter
  4. environ can use a dict-comprehension for brevity and speed to avoid repeated update calls

Do those make sense? @lexming @boegel

Automatically do the detection if not done yet to avoid forgetting it.
@boegel boegel added this to the 5.0 milestone Feb 5, 2025
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 df528c6 into easybuilders:5.0.x Feb 5, 2025
39 checks passed
@Flamefire Flamefire deleted the dict-improve branch February 5, 2025 09:06
@lexming
Copy link
Contributor

lexming commented Feb 5, 2025

@Flamefire yeah, those make perfect sense, thanks for the PR!

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.

3 participants