Tolerate absolute filenames?

[coreyfarrell]

nyc has gotten a report that --nycrc-path does not support absolute paths - true as we use find-up to find the requested configuration file. Currently if CWD is /home/coreyfarrell/test-project and I run findUp.sync('/home/coreyfarrell/nycrc.json', {cwd: process.cwd()}), this will return /home/coreyfarrell/test-project/home/coreyfarrell/nycrc.json. This would be fixed if find-up used path.resolve instead of path.join. Would you be open to this? I can post an implementation including tests.

[sholladay]

This is already fixed in #28. Feel free to use the fork until we merge it. :)

That said, IMO passing absolute paths to find-up is a bit confusing because it's not clear whether find-up should return the path as-is or whether it will do a search for the filename of the leaf. And if it does a search, does the directory from the absolute path take precedence over the cwd option? It's all a little murky unless you dig into it.

I would recommend short-circuiting the logic to explicitly use the given filepath as-is if it is absolute, for clarity's sake, avoiding find-up altogether.

[sindresorhus]

That said, IMO passing absolute paths to find-up is a bit confusing because it's not clear whether find-up should return the path as-is or whether it will do a search for the filename of the leaf. And if it does a search, does the directory from the absolute path take precedence over the cwd option? It's all a little murky unless you dig into it.

Let's document it. I think find-up should return the path as-is if it exists.

[coreyfarrell]

@sholladay Yes it looks like your PR will resolve this for us in nyc@15 (that'll be when we drop node.js 6). Do you want to add Fixes #36 to your PR?

[sholladay]

Let's document it. I think find-up should return the path as-is if it exists.

Sure, that's reasonable. But what if it doesn't exist at the given path? Do people expect us to immediately return undefined or to search for the basename of that filepath (i.e. what this module is designed to do) under the assumption that the basename is really what the user cares about? Does the nyc team have an opinion on that @coreyfarrell? I think I lean towards the former, but it's worth discussing as I can see people misunderstanding it from glancing at code (without reading the docs).

[coreyfarrell]

Other than using path.resolve instead of path.join I don't think findUp should make any special consideration for absolute filenames. Otherwise it would complicate handling of situations like findUp(['/tmp/absolute/filename', 'relative-filename']).

If find-up doesn't find an absolute filename in cwd then it won't find it in path.dirname(cwd), eventually it'll return undefined. I don't think it's very important that cycles are wasted searching up the path for the same absolute filename, this is an edge case and IMO not worth optimizing at the expense of code clarity.

Unrelated but for completeness of discussion, find-up does support finding relative filenames. For example you could have /usr/src/test-project/.github/ISSUE_TEMPLATE and run findUp('.github/ISSUE_TEMPLATE', {cwd: '/usr/src/test-project/test'}). This will find the file one directory up from cwd.

I've opened sholladay#1 to add tests to cover edge cases.

[sindresorhus]

Other than using path.resolve instead of path.join I don't think findUp should make any special consideration for absolute filenames.

👍

I don't think it's very important that cycles are wasted searching up the path for the same absolute filename, this is an edge case and IMO not worth optimizing at the expense of code clarity.

👍