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 JsHintReporter _builtInIgnoreRegex #635

Merged
merged 1 commit into from
Feb 8, 2014

Conversation

ChrisTorng
Copy link
Contributor

This is my first time to contribute to open source project. Everything to me is new. Like GitHub, git, VS SDK. Even I'm not good at Regex too. If I do anything wrong, please forgive me.

I just looking for #603, found JSHint ignore rule mention by @SLaks.
All '.' char in Regex require ''.
I have tried to find a test case about this, but not exists. And I have no ability to create a new one. Hope this commit is right.

All '.' char in Regex require '\'.
madskristensen added a commit that referenced this pull request Feb 8, 2014
Fix JsHintReporter _builtInIgnoreRegex
@madskristensen madskristensen merged commit b9ad2d5 into madskristensen:master Feb 8, 2014
@madskristensen
Copy link
Owner

Thanks!! And congrats on your first pull request :)

@ChrisTorng
Copy link
Contributor Author

Really happy to be accept so fast. ;) The next WE will have my contribution, used by lots of people. This is the first time I get into this! Thanks for all your awesome work!

@@ -56,19 +56,19 @@ public static bool ShouldIgnore(string file)
@"dojo\.js",
@"ember\.js",
@"ext-core\.js",
@"handlebars.*",
@"handlebars\.*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this is wrong; \.* will only match repetitions of .s

@ChrisTorng
Copy link
Contributor Author

Oops!!! My first contribute is totally wrong, due to my poor Regex! Should I revert it all back? Or whoever found problem will do it (just like what I do)? I don't know the common rule for resolving this.

@madskristensen
Copy link
Owner

Just send a PR with the change

@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

@madskristensen This was fixed by #637

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.

3 participants