Skip to content

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