Skip to content

Conversation

@boegel
Copy link
Member

@boegel boegel commented Oct 25, 2020

No description provided.

@boegel boegel added this to the 4.x milestone Oct 25, 2020
@boegel boegel modified the milestones: 4.x, 4.3.2 Oct 25, 2020
Comment on lines +795 to +798
def locate_files(files, paths, ignore_subdirs=None):
"""
Determine full path for list of files, in given list of paths (directories).
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the code seem to be strictly targeted at easyconfig files, the name and description should say so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing in this function is specific to easyconfigs. I've renamed the ec_file local variable to filepath, maybe that made you reach that conclusion?

# try to load index for current path, or create one
path_index = load_index(path, ignore_dirs=ignore_subdirs)
if path_index is None or build_option('ignore_index'):
if os.path.exists(path):
Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably be better to have before the log_debug, i.e. first check if the path exists at all and if not just skip it completely, i.e. "if not os.path.exists(path): next" or however that is done in python

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good point, fixed in bb18de4

@boegel
Copy link
Member Author

boegel commented Nov 17, 2020

@akesandgren Tackled or replied to remarks, please re-review

Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren
Copy link
Contributor

Going in, thanks @boegel!

@akesandgren akesandgren merged commit 000e79e into easybuilders:develop Nov 20, 2020
@boegel boegel deleted the locate_files branch November 20, 2020 19:43
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