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

fix: watch does not work for glob pattern which intersects with non-glob part of another glob pattern #748

Closed
wants to merge 1 commit into from

Conversation

eGavr
Copy link

@eGavr eGavr commented Jul 14, 2018

Resolves #747

@coveralls
Copy link

coveralls commented Jul 14, 2018

Coverage Status

Coverage increased (+0.06%) to 96.271% when pulling a056059 on eGavr:issue/747 into 8cd5f7a on paulmillr:master.

@srguiwiz
Copy link
Contributor

srguiwiz commented Jul 15, 2018

@eGavr , thank you for taking initiative and fixing.

Testing is necessary. Testing this project for now is a little bit odd, because the test suite has cases in it that don't pass, for yet to be fixed defects.

In order to ensure not to break existing functionality with a next release, please:

  • Run test suite with your fix not in (previous commit), note what fails.
  • Run test suite with your fix. Compare what fails then.

That protects quality and sanity.

To help prevent future regression, you could also add a test case in test.js that fails before your fix and then passes with your fix.

@eGavr eGavr changed the title fix: watch does not work for glob pattern which intersects with another glob pattern fix: watch does not work for glob pattern which intersects with non-glob part of another glob pattern Jul 16, 2018
@eGavr
Copy link
Author

eGavr commented Jul 16, 2018

@srguiwiz

I've created a testing pull request #749 where I've just changed README and you can see in it that there are 7 broken tests.

In my PR there were 7 broken tests too.
Besides, I think that some tests are not stable and fail randomly.

Can you review my changes, please? It seems that they do not break the current functionality and fix the problem which I've described in the issue.

@eGavr
Copy link
Author

eGavr commented Jul 17, 2018

ping :)

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.

None yet

4 participants