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

Fixed Path to allow folders, as already documented #495

Merged
merged 2 commits into from
Jun 28, 2021
Merged

Conversation

jbednar
Copy link
Member

@jbednar jbednar commented Jun 27, 2021

Path is documented as accepting both files and folders (directories), but it was only accepting files. Fixed to make Path check only that the path exists, not whether it is a file or folder. Also simplified the logic overall. Marked API since the behavior has apparently never matched the documented API; now it should match the previously documented API.

@jbednar jbednar added the API Improvement to API or something like that label Jun 27, 2021
@jbednar jbednar requested a review from jlstevens June 27, 2021 23:39
@jbednar jbednar merged commit 382746c into master Jun 28, 2021
@jbednar jbednar deleted the pathfolder branch June 28, 2021 00:16
@@ -1628,45 +1628,39 @@ class resolve_path(ParameterizedFunction):
Prepended to a non-relative path, in order, until a file is
found.""")

path_to_file = Boolean(default=True, pickle_default_value=False, doc="""
path_to_file = Boolean(default=True, pickle_default_value=False,
allow_None=True, doc="""
String specifying whether the path refers to a 'File' or a 'Folder'.""")
Copy link
Contributor

@jlstevens jlstevens Jul 5, 2021

Choose a reason for hiding this comment

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

I was wondering how it could be anything other than True or False but looking at the code I see that None means either a file or a directory. I'll clarify this docstring further in a small PR.

Minor nitpick but I associate 'folder' with the Windows world while I consider 'directory' to be the proper platform-independent terminology. Probably not worth updating as everyone knows what you mean!

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarified docstring in #507. I won't mark this as resolved in case you have any opinion on 'Folder' versus 'Directory'.

Copy link
Member Author

@jbednar jbednar Jul 5, 2021

Choose a reason for hiding this comment

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

Seems like the docstring is still confusing as it is a boolean, not a string as claimed. Thanks for the None clarification. I agree with your preference for "directory" over "folder", but I am just following the existing param.Filename and param.Foldername terminology.

@jlstevens jlstevens mentioned this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Improvement to API or something like that
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants