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 filter option #66

Merged
merged 15 commits into from Mar 7, 2020
Merged

Add filter option #66

merged 15 commits into from Mar 7, 2020

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented Jun 18, 2019

  • Add filter option

Fixes #63


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@stroncium
Copy link
Contributor Author

Windows tests not passing, but not affected by this patch.

@sindresorhus
Copy link
Owner

I need your feedback on #48 (comment). I think we could use the idea here too. basename is too limiting, especially when deciding to filter something.

index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@stroncium
Copy link
Contributor Author

stroncium commented Jun 28, 2019

@sindresorhus Well, I reworked it to work like #48 (comment) .
Changed property names a bit though.

@sindresorhus
Copy link
Owner

1955af9 is not really what I proposed in #48 (comment).

@stroncium
Copy link
Contributor Author

@sindresorhus What part do you mean? I read it again and the only differences I found are:

  • no Object.freeze() (but do we really need it?)
  • a bit different field names
  • it'sfilter, not rename

Though I didn't understand what can Proxy be used for (in filter nor in rename).

It might be that either you failed to describe the design you see or I completely failed at understanding it.

@sindresorhus
Copy link
Owner

no Object.freeze() (but do we really need it?)

Yes. I like to be defensive. It's too easy to make mistakes in JS.

a bit different field names

I'm fine with that, except resolvedPath. .path should be the resolved path, but we can also include a .cwd property (?)

it's filter, not rename

Sure, but the intention is to unify them so they both use the exact same kind of "file" object.

Though I didn't understand what can Proxy be used for (in filter nor in rename).

That's only really useful for rename, but it allows you to update any of the properties and have the change reflected in all the other properties.

It might be that either you failed to describe the design you see or I completely failed at understanding it.

Probably me not being clear enough. If I'm ever not clear enough, please let me know.

@sindresorhus
Copy link
Owner

I've seen nameWithoutExtension called stem in some places, but I'm not sure it's common knowledge enough to be worth the unclarity.

@stroncium
Copy link
Contributor Author

stroncium commented Jul 3, 2019

re: Proxy
That behavior is normally implemented in JS by getters/setters, Proxy is more for debugging(like logging all accesses to a field) or decorating abstract external objects when you don't know what the fields will be.
Implementing this by proxy will require more code than getters/setters and also more complicated code(basically a copy of getters/setters code plus code to make getters/setters out of proxy).
Also, https://jsperf.com/proxy-vs-getset-vs-function/1 , proxies are 30-300 times slower than getters/setters.

@sindresorhus
Copy link
Owner

You’re right. Getters/setters is a better choice in this case.

@stroncium
Copy link
Contributor Author

stroncium commented Jul 25, 2019

@sindresorhus Are you sure about resolved path vs non-resolved path? As cpy operates in "scope" of cwd, users probably won't care about full resolved path for filtering purposes. In any case, including cwd seems a bit weird to me, as user can get it himself and it's the same for all files.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I agree the resolved path is not that useful in a filtering purpose. However, it's good to have it for consistency with the file object that will be used in the rename option. And also, there are scenarios where you'd want the full path. For example, when you filter based on files outside the cwd. I think path should be the resolved path as that's the expected behavior. We can also add a relativePath property with is made relative based on cwd, however, it should not be directly the path the user provided as that might be relative or absolute.

@sindresorhus
Copy link
Owner

Ping :)

index.js Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

We can also add a relativePath property with is made relative based on cwd, however, it should not be directly the path the user provided as that might be relative or absolute.

@sindresorhus
Copy link
Owner

@stroncium Bump :)

@stroncium
Copy link
Contributor Author

@sindresorhus added relativePath

index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 7df88b8 into sindresorhus:master Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add filter option
2 participants