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 regexp for IgnorePlugin #4418

Closed
wants to merge 2 commits into from

Conversation

Dean-Coakley
Copy link
Contributor

@Dean-Coakley Dean-Coakley commented Mar 3, 2017

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
No not yet, not sure how. I

Summary
contextRegExp should be checked if it is provided in both cases: normal-module-factory and context-module-factory.

Other information
Should fix: #4415

@jsf-clabot
Copy link

jsf-clabot commented Mar 3, 2017

CLA assistant check
All committers have signed the CLA.

@Dean-Coakley
Copy link
Contributor Author

@TheLarkInn This is my first contribution. Sorry if I am making some mistakes.

Does this require some test addition?

Is two commits ok or do I need to rebase into one?

@barinali
Copy link
Contributor

barinali commented Mar 4, 2017

@Dean-Coakley I think it's okay. Because it has test already. I think you didn't change anything that needs new test cases if I'm not wrong.

Besides that, two commits are welcome.

@sokra
Copy link
Member

sokra commented Mar 5, 2017

You can add a test in test/configCases.

@sokra
Copy link
Member

sokra commented Mar 7, 2017

Let us know if you have problems with creating a test case.

@Dean-Coakley
Copy link
Contributor Author

Yes, I will not be able to write a test without a lot of help. - No experience of writing any unit tests or anything. Very open to learning despite my lack of free time though.

Sorry about this.

@TheLarkInn
Copy link
Member

First could I convince you to read test/README.md, and see if that helps? I just explains what all the different cases do and how to add one. And then I can personally help you through adding a new one if it still isn't clear.

@TheLarkInn
Copy link
Member

Let me see if I can give you an example to work with today.

@Dean-Coakley
Copy link
Contributor Author

Dean-Coakley commented Mar 19, 2017

Might be difficult for me to find time to work on this for the next few days, but I'd love if you could give me some guidance.

Thanks!

@webpack-bot
Copy link
Contributor

The minimum test ratio has been reached. Thanks!

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

The code looks good, but it would be really nice to have one simple test for this.

See test/README.md for help and add a testcase to test/configCases/...

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@timse
Copy link
Member

timse commented Apr 9, 2017

finished through #4669
hope thats ok @Dean-Coakley

@timse timse closed this Apr 9, 2017
@Dean-Coakley
Copy link
Contributor Author

Yup, all good @timse . I don't think I would have figured that out anyway. Thanks.

@Dean-Coakley Dean-Coakley deleted the IgnorePluginHotfix branch April 26, 2017 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IgnorePlugin doesn't check contextRegExp for context-module-factory
7 participants