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

feat: add fs/resolve-parent-paths #2566

Merged
merged 33 commits into from
Jul 21, 2024

Conversation

Snehil-Shah
Copy link
Member

Resolves #2565

Description

What is the purpose of this pull request?

This pull request:

  • adds the package @stdlib/fs/resolve-parent-paths

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@kgryte
Copy link
Member

kgryte commented Jul 12, 2024

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.

@Snehil-Shah
Copy link
Member Author

Snehil-Shah commented Jul 12, 2024

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.
Someone for example might be searching for project configuration by either resolving a yarn.lock or a package-lock.json, or a requirements.txt or a requirements.in file, and would be expecting a resolved path as output with whatever matches first.

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?

@Snehil-Shah
Copy link
Member Author

Snehil-Shah commented Jul 12, 2024

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 package.json and package-lock.json together

@Snehil-Shah
Copy link
Member Author

But a user might want to strictly search? Meaning only if all paths match, return?

@kgryte
Copy link
Member

kgryte commented Jul 12, 2024

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 package.json and package-lock.json together

Yes, that was the use case I was thinking about.

But a user might want to strictly search? Meaning only if all paths match, return?

Yes, that is also possible.


In short, looks like there are 4 potential "modes":

  1. return the first match
  2. return one or more matches upon finding a directory containing a match
  3. return paths only when all files are present in a directory
  4. walk filesystem finding first matches as we go, only stopping once all files have found a match (meaning, the returned paths need not be in the same directory; this is the equivalent of calling resolve-parent-path twice)

There's a fifth possibility which is returning all parent paths satisfying a provided path, but this is a different package (e.g., resolve-all-parent-paths).

Currently, this PR implements (1). I am fine with that. Maybe, however, we should rename the package to @stdlib/fs/resolve-first-parent-path to convey that it's a race to find the first match from a list of candidates, similar, conceptually, to Array.prototype.find (i.e., find the first path satisfying a set of path filters).

WDYT?

@Snehil-Shah
Copy link
Member Author

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?

@kgryte
Copy link
Member

kgryte commented Jul 12, 2024

That is also fine. I don't have a strong opinion here.

@Snehil-Shah
Copy link
Member Author

@kgryte let's stick with a mode option. Should I add different modes in future PRs or update this one?

@kgryte
Copy link
Member

kgryte commented Jul 12, 2024

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.

@Snehil-Shah
Copy link
Member Author

Snehil-Shah commented Jul 12, 2024

  1. walk filesystem finding first matches as we go, only stopping once all files have found a match (meaning, the returned paths need not be in the same directory; this is the equivalent of calling resolve-parent-path twice)

Do we really need this mode, a user can simply call resolve-parent-path twice instead within their own logic?

And also, what should the names of the modes and the default be? Would appreciate some suggestions..

@kgryte
Copy link
Member

kgryte commented Jul 12, 2024

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:

  • first: first match
  • some: return one or more paths from the same level
  • all: only return paths if all paths match within a level
  • each: return resolved paths independently

Those work?

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. labels Jul 13, 2024
@Snehil-Shah
Copy link
Member Author

Should all be the default mode? I think a user is more likely to want to get both paths together when using the package without any mode configuration..?

@kgryte
Copy link
Member

kgryte commented Jul 14, 2024

Yes, that seems reasonable.

@Snehil-Shah
Copy link
Member Author

Snehil-Shah commented Jul 14, 2024

Always returning an array raises some questions.

  • in first mode, if the 2nd element in the input array was resolved, we return the path in an array with one element only ie [path]. The user might be expecting the output to be [null, path].

  • in some or each mode, if say only the 3rd element was resolved, we return [null, null, 3], but if no element was resolved we return an empty array []. The user might be expecting an array of nulls.

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 all mode, it still makes sense as it either returns all paths or an empty array

@kgryte
Copy link
Member

kgryte commented Jul 14, 2024

@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 null values from the results seems less than ideal. For first, I am having a hard time imagining a scenario where you'd care about which file did not get resolved first. By specifying first, a user is saying give me the first thing that matches, so returning null values for other candidates seems off.

Ditto for some. I would not expect [ null, null, 3 ]. I just expect [3]. Again, this is "give me the first matches", and not clear that a user needs to know what didn't get matched. A user is still able to get this info. They just need to post-process [3] by iterating over the results and doing re.test( paths[ i ] ) === false.

For both first and some, I'd expect an empty array if no matches are found.

For each, returning null for unresolved paths makes sense. So [ null, null, null ] works when no path is resolved.

For all, returning an empty array when no path is resolved and [ path, path, path ] when all paths match seems fine.

@Snehil-Shah
Copy link
Member Author

@kgryte Should we break it down into separate packages, if it's turning a bit messy?

@kgryte
Copy link
Member

kgryte commented Jul 14, 2024

I think this should be fine, as is. We can always refactor out later if it turns messier. 😅

@kgryte
Copy link
Member

kgryte commented Jul 14, 2024

Summarizing:

  1. first: [ path ] or []
  2. some: [ path, path, ... ] or []
  3. each: [ path|null, path|null, path|null ]
  4. all: [ path, path, path ] or []

@kgryte
Copy link
Member

kgryte commented Jul 14, 2024

That seems consistent enough.

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah Snehil-Shah requested a review from kgryte July 14, 2024 22:05
@kgryte kgryte added the Needs Review A pull request which needs code review. label Jul 14, 2024
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah Snehil-Shah requested a review from kgryte July 16, 2024 19:09
@kgryte kgryte changed the title feat: add @stdlib/fs/resolve-parent-paths feat: add fs/resolve-parent-paths Jul 18, 2024
@kgryte kgryte added Needs Review A pull request which needs code review. and removed Needs Changes Pull request which needs changes before being merged. labels Jul 18, 2024
@Planeshifter Planeshifter self-requested a review July 18, 2024 14:04
@kgryte kgryte removed the Needs Review A pull request which needs code review. label Jul 19, 2024
Copy link
Member

@kgryte kgryte left a 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.

Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

LGTM!

@Planeshifter Planeshifter merged commit f5f09ad into stdlib-js:develop Jul 21, 2024
8 checks passed
gunjjoshi pushed a commit to gunjjoshi/stdlib that referenced this pull request Aug 21, 2024
PR-URL: stdlib-js#2566 
Closes: stdlib-js#2565

---------

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Co-authored-by: Athan Reines <kgryte@gmail.com>
Reviewed-by: Athan Reines <kgryte@gmail.com> 
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: add @stdlib/fs/resolve-parent-paths
3 participants