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

Add findUpMultiple() method #27

Closed
silverwind opened this issue Sep 24, 2018 · 20 comments · Fixed by #56
Closed

Add findUpMultiple() method #27

silverwind opened this issue Sep 24, 2018 · 20 comments · Fixed by #56
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@silverwind
Copy link

silverwind commented Sep 24, 2018

Issuehunt badges

I have a use case where I need to find all matching files in parent paths and I wonder if you would consider having a multiple option that causes the function to return a array of filepaths.

I may also need to stop at a certain path (e.g. don't go all the way to root), so a stopat option might also be nice to have. handled by findUp.stop.


IssueHunt Summary

vast vast has been rewarded.

Backers (Total: $60.00)

Submitted pull Requests


Tips

@silverwind silverwind changed the title multiple option multiple and stopat options Sep 24, 2018
@sholladay
Copy link
Collaborator

I think you will find the discussion in #19 to be helpful.

In short: we plan to implement #21 instead and then you can write your own accumulator function that decides when to stop. See the example code in the linked issues for how it would be used.

I'm going to close this, since that should solve your use case quite well. Would love to hear your thoughts, though!

I will set aside some time over the weekend to get this done. Seems like a lot of people need it.

@silverwind
Copy link
Author

Okay, guess it's time that I write my own implementation then.

@sindresorhus
Copy link
Owner

Even with #21, I think these options would be useful as high-level options for the common-case.

The multiple option is just about exposing information we already have. That way the user can easily get, for example, the last two paths. I would name the option all.

The stopat option solves a common use-case of needing to stop at a certain point, usually the root project directory or the home directory. This option will need a better name though.

@sindresorhus sindresorhus reopened this Sep 25, 2018
@sholladay
Copy link
Collaborator

There was already a PR to add the stopAt functionality. I should have linked to that. Here it is: #18

IMO the function approach is fairly elegant for this use case. Let me try to clarify with a usage example. Below, I create a list of package.json files, stopping at a directory named abcd.

const stopDir = path.resolve('abcd');
const matchingFiles = [];
await findUp((dir) => {
    const filePath = path.join(dir, 'package.json');
    if (fs.existsSync(filePath)) {
        matchingFiles.push(filePath);
    }
    return dir === stopDir;
});

@silverwind
Copy link
Author

silverwind commented Sep 26, 2018

This option will need a better name though.

How about start and stop? start could be an alias for the current cwd, or just rename the option in a major bump (I don't particularily like cwd, so I hope you agree to remove it).

@silverwind
Copy link
Author

silverwind commented Sep 26, 2018

@sholladay even if it can be done with a function argument, I'd say this is a pretty commonly needed option. I'd probably be willing to PR both if @sindresorhus approves.

@sholladay
Copy link
Collaborator

In case anyone comes here looking for this, I have implemented the function approach. You can use my branch for now until it's released (see PR #28).

@sindresorhus
Copy link
Owner

Even with #28, I still think these options would be useful.

I don't think stop is a descriptive enough name. How about rootDirecory or stopAtDirectory? Other suggestions?

As for the all option. I think it might be better to make it a separate method as it's nicer to have separate methods when they return different things. That also simplifies the TS definition. So it could be an .all() or .multiple() method. (Which?)

@sholladay
Copy link
Collaborator

I like .all() or .every(). If we add that, it might make sense to move the existing array signature to .some().

@sindresorhus
Copy link
Owner

I like .all() or .every().

Let's go with .all() then.

If we add that, it might make sense to move the existing array signature to .some().

Do you mean the array input? If so, that would be weird, as making .all() a separate method is because the return value type is different. Array as input in findUp() doesn't change the return value type.

@sholladay
Copy link
Collaborator

Ah, right. I forgot these key words from the OP:

return a array of filepaths

I've never needed anything like that. In my head, I was thinking that findUp.multiple([fileA, fileB]) would return a single result: the path of the directory that contains all of those files. That's a much more common need for me. For example, I want to search until I find the directory that contains both package.json and node_modules. Hence why I referred to it as .every(). I see now that .all() is a much more sensible name for what this issue is about, as it's akin to Promise.all().

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 2, 2019
@IssueHuntBot
Copy link

@IssueHunt has funded $60.00 to this issue.


@sindresorhus
Copy link
Owner

For example, I want to search until I find the directory that contains both package.json and node_modules.

This could potentially also be an option to just findUp() (since it still returns only one path) that decides whether the input array means to find the first found path in the array or when it finds all of them. Or do you think that would be confusing?

@sindresorhus
Copy link
Owner

// @coreyfarrell In case you have any thoughts.

@coreyfarrell
Copy link
Contributor

For example, I want to search until I find the directory that contains both package.json and node_modules.

locate-path does not currently support matching multiple paths (it stops at the first match). Would it be better to expand that module or implement a replacement matcher function here? One issue is that locate-path no longer supports searching for files and directories so this could be a problem if you wanted to find package.json and node_modules. I suppose this could be a use case for type: undefined.

For the ability to return an array of files I think a separate findUp.multiple makes sense where findUp.multiple(['file1', 'file2']) would return an array of all matching paths when one or more match. .all or .every might be confusing given the specific meaning for Array's / Promises.

@silverwind
Copy link
Author

Regarding the initial request for a stopat option, I found it's already possible via findUp.stop, so I'm dropping that request for it from this issue.

@silverwind silverwind changed the title multiple and stopat options multiple option Oct 13, 2019
@sholladay
Copy link
Collaborator

The stop option could still be useful for people using names instead of matcher functions. But yeah, it seems like a rare use case to me and it barely saves the user any keystrokes.

@silverwind
Copy link
Author

silverwind commented Oct 13, 2019

Combining matcher and findUp.stop is essentially what a stop argument would do, thought it's not as straightfoward to use as a simple argument would be. Here's how I implemented it. Not particularily happy about having to do that length check to decide when to stop.

Anyways, I think we should have one issue per feature request, so I'd say we should open another for stopat.

@sindresorhus sindresorhus changed the title multiple option Add .multiple() method Feb 14, 2020
@sindresorhus
Copy link
Owner

sindresorhus commented Feb 14, 2020

So in summary: It should be a findUpMultiple() method (named export) that is like findUp() but returns one or more matches as an array instead of just the first match.


Note: To get the bounty, you need to fix #49 too.

@sindresorhus sindresorhus changed the title Add .multiple() method Add findUpMultiple() method Sep 26, 2021
@issuehunt-oss
Copy link

issuehunt-oss bot commented Oct 7, 2021

@sindresorhus has rewarded $54.00 to @vast. See it on IssueHunt

  • 💰 Total deposit: $60.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $6.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants