Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 support for path matcher function #28
Add support for path matcher function #28
Changes from 1 commit
95d4390
8b80cf1
e8072bb
44e67bf
102b688
7a83413
6ad0dd8
ca48c76
f858bdd
eff8479
cba6f26
af9d361
b10d5b2
eb97a2f
ae4d8bc
bb10d62
94fafd1
41b8bdb
cf3157c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think it would be useful for us to at least expose convenience methods to check whether the path exists in a normalized way.
fs.access()
is not obvious to use and you would have to manually promisify it when using it async.We already depend on
path-exists
inlocate-path
, so we could depend on it and expose it here somehow without any increase in dependencies. I think this makes sense as checking whether the path exist is the most common operation you would do.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.
I think my documentation made it sound worse than it is.
fs.access()
isn't really necessary. In most use cases forfind-up
, it's sufficient to usefs.existsSync()
if the filesystem even needs to be checked, and that returns a simple boolean, making it much easier to use here. I've added an example of that to the Usage section instead. Hopefully, a little simpler/clearer now.I did consider that maybe we should ignore the result of the matcher if the filepath it returns does not exist. But I think it's highly likely that the matcher will be checking the filesystem anyway. And by leaving this up to the matcher, we also enable the user to decide where the search stops in a more deterministic manner (i.e. its decision will never be overridden). I think this is a good foundation to build on top of.
As for convenience methods, are you thinking something like
findUp.exists(matcher)
/findUp.existsSync(matcher)
? Or maybe a pipeline likefindUp(matcher, findUp.exists)
? My immediate reaction is this merely saves a few characters. If we were to add this, I'd prefer the latter pipeline pattern.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.
Can you use the
directory
wording for all these instead ofdir
?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.
Done.