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

simplify code for determining the PYTHONPATH module entries #4686

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

Flamefire
Copy link
Contributor

The check whether the path(s) have already been added is redundant as that is done in the module_generator class.

The whole code with all the checks is superflous if there are no python paths to add.

To avoid excessive indentation return early with an empty list in that case and same for Python installations for consistency.

prepend_paths can return an empty string when everything is filtered. As we return a list here (we might be better of returning a string instead) return an empty list to avoid empty lines being written if '\n' was used. -> TODO for later

Best to view with whitespace changes ignored

@bartoldeman bartoldeman changed the title simplify code for determining the PYTHONPAH module entries simplify code for determining the PYTHONPATH module entries Oct 23, 2024
@Micket
Copy link
Contributor

Micket commented Nov 1, 2024

The check whether the path(s) have already been added is redundant as that is done in the module_generator class.

which then also prints warnings, which i wanted to avoid

https://github.com/easybuilders/easybuild-framework/blob/b5c1dd97fd34bb15855783002b2874184f1987b1/easybuild/tools/module_generator.py#L231C1-L233C40

@boegel boegel added the EasyBuild-5.0 EasyBuild 5.0 label Nov 6, 2024
@boegel boegel added this to the 5.0 milestone Nov 6, 2024
@Flamefire
Copy link
Contributor Author

which then also prints warnings, which i wanted to avoid

Oh I see. I'd avoid to duplicate the logic though, see the new commit(s)

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

actually, while attempting to solve the merge conflict, I think we should also add the new warn_exists keyword to append_paths as well, to make them similar.

Flamefire and others added 4 commits November 18, 2024 12:56
The check whether the path(s) have already been added is redundant as that is done in the module_generator class.

The whole code with all the checks is superflous if there are no python paths to add.

To avoid excessive indentation return early with an empty list in that case and same for Python installations for consistency.
@Flamefire
Copy link
Contributor Author

Good point, rebased and implemented

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

lgtm

@Micket Micket merged commit 6db0a64 into easybuilders:5.0.x Nov 22, 2024
39 checks passed
@Flamefire Flamefire deleted the patch-1 branch November 22, 2024 16:55
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