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 new negatedExtglob state to result of scan #79

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Conversation

danez
Copy link
Member

@danez danez commented Apr 12, 2021

This allows consumers to see if the glob starts with a negated extglob similar to parse.
At first I wondered if this should maybe set negated to true, but that might be a breaking change and also is then completely different from parse.

One example is jest which uses scan and matcher to mimic micromatchs main api. https://github.com/facebook/jest/blob/d49cb30fb2540efa8c451fb6cb24e8997284bc67/packages/jest-util/src/globsToMatcher.ts#L49-L52

This makes it possible to solve this issue micromatch/micromatch#179 in jest as well as basically support all negated extglobs as non of them work correctly atm.

    This allows consumers to see if the glob starts with a negated extglob similar to parse.
    At first I wondered if this should maybe set negated to true, but that might be a breaking change and also is then completely different from parse.
@danez danez added the enhancement New feature or request label Apr 12, 2021
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 92.303% when pulling c1950a5 on extglob-scan into 8839c01 on master.

Copy link
Member

@jonschlinkert jonschlinkert left a comment

Choose a reason for hiding this comment

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

Nice work! As you noted this does not catch negated extglobs when they are not the first thing in the glob. Perhaps we should also detect non-starting negated extglobs as well? Or we could change the flag name to negatedStartingExtglob or something like that. Otherwise this might be misleading. Thoughts?

lib/scan.js Show resolved Hide resolved
@danez
Copy link
Member Author

danez commented Apr 12, 2021

Nice work! As you noted this does not catch negated extglobs when they are not the first thing in the glob. Perhaps we should also detect non-starting negated extglobs as well? Or we could change the flag name to negatedStartingExtglob or something like that. Otherwise this might be misleading. Thoughts?

I tried to align the property with the state of parse for consistency and in parse it is also only set if it is at the beginning.
I can rename it to negatedStartingExtglob, but then I would guess it should be also added to parse and deprecate negatedExtglob there?

@jonschlinkert
Copy link
Member

I tried to align the property with the state of parse

Good point, and that reminds me that the point of this is feature detection, so it's okay to simply return true if that feature exists. Thanks for discussing, it's helping me remember finer details about the code.

Really nice work! Feel free to merge and publish.

@danez
Copy link
Member Author

danez commented Apr 12, 2021

I will first try to propose a different change to jest, swapping mm.scan() + mm.matcher() for picomatch(globs, {}, returnState: true). This eliminates scan completely as picomatch can return the state of parse anyway.

@danez
Copy link
Member Author

danez commented Apr 12, 2021

Okay we went with picomatch directly in jest, but I guess this is still a valuable feature, so merging.

@danez danez merged commit ce64fd1 into master Apr 12, 2021
@danez danez deleted the extglob-scan branch April 12, 2021 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants