-
-
Notifications
You must be signed in to change notification settings - Fork 820
feat: add fs/resolve-parent-paths
#2566
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
feat: add fs/resolve-parent-paths
#2566
Conversation
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
I wonder if this should actually return one or more paths. Currently, it returns whatever matches first. But what if you want all matching paths? Maybe we should always return an array and the function should stop walking dirs until the first match is found. Once found, the function returns all matching paths in that directory. |
I think the use-case of the API would be similar to what we want it for, that is, resolve a path from a list of possible paths I allow. I am not sure the aim/use-case of the API is clear to me with what you're proposing. What should the updated API description be? |
Wait now that I think again, a user might want to resolve many paths at once at a directory level. For example: a user might want to search both |
But a user might want to strictly search? Meaning only if all paths match, return? |
Yes, that was the use case I was thinking about.
Yes, that is also possible. In short, looks like there are 4 potential "modes":
There's a fifth possibility which is returning all parent paths satisfying a provided path, but this is a different package (e.g., Currently, this PR implements (1). I am fine with that. Maybe, however, we should rename the package to WDYT? |
I was thinking if it would be better to have different modes as options to this one package instead of having individual packages for each mode? |
That is also fine. I don't have a strong opinion here. |
@kgryte let's stick with a |
I suggest going ahead and adding to this PR. I'd also always return an array, even if only returning a single path. For the case of no matches, an empty array. |
Do we really need this mode, a user can simply call And also, what should the names of the modes and the default be? Would appreciate some suggestions.. |
Calling twice would incur additional perf overhead as then need to walk file tree two separate times rather than once. In general, we could say the same about your use case. You could have just called resolve-parent-path twice, if your first attempt as a json file failed. Modes:
Those work? |
Should |
Yes, that seems reasonable. |
Always returning an array raises some questions.
Should we instead always return an array of same length as the original array (null where no resolution could happen), just so the resolved paths are always mapped to the original path names? But then in |
@Snehil-Shah This API has become a bit of a rabbit hole of design questions, which, to me, is a code smell. It seems hard to cater nicely to each of the different "modes", which suggests independent functionality, rather than overloading a single API. This stated, a user needing to filter out Ditto for For both For For |
@kgryte Should we break it down into separate packages, if it's turning a bit messy? |
I think this should be fine, as is. We can always refactor out later if it turns messier. 😅 |
Summarizing:
|
That seems consistent enough. |
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
Signed-off-by: Snehil Shah <[email protected]>
@stdlib/fs/resolve-parent-paths
fs/resolve-parent-paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll let @Planeshifter perform a final review to see if I missed anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
PR-URL: stdlib-js#2566 Closes: stdlib-js#2565 --------- Signed-off-by: Snehil Shah <[email protected]> Co-authored-by: Athan Reines <[email protected]> Reviewed-by: Athan Reines <[email protected]> Reviewed-by: Philipp Burckhardt <[email protected]>
Resolves #2565
Description
This pull request:
@stdlib/fs/resolve-parent-paths
Related Issues
This pull request:
@stdlib/fs/resolve-parent-paths
#2565Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers