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

Combine JsHint and JSCS rule, hack to stop errors #637

Merged
merged 6 commits into from
Feb 10, 2014

Conversation

ChrisTorng
Copy link
Contributor

Combine JsHint and JSCS rule and cleanup excludeFiles. Hack to stop <xml
and more than 500 errors. Revert Regex.

Going to go out, I will write more detail about these changes later.

Combine JsHint and JSCS rule and cleanup excludeFiles. Hack to stop <xml
and more than 500 errors. Revert Regex.
@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

Thank you; this is exactly what we need to do.

For the future, can you try to keep unrelated changes in separate commits?
You can push multiple commits and they will all be included in the same pull request.

@ChrisTorng
Copy link
Contributor Author

I don't know anything left need to be done with a quick fix. Except for others testing. You all have checked all my commits. Are there anything left? I have one more free day to deal with it. Later I won't have much time to contribute...

@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

This look great; thanks!

@nschonni
Copy link
Contributor

nschonni commented Feb 9, 2014

I'm confused by the rename of JsHintReporter -> JsLintReporter since they are separate projects that have diverged greatly over the years. It doesn't seem like there is a practical reason for the rename and it might confuse people into thinking that JSLint is the project being used.

@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

@nschonni Yes; this is confusing.

The name now does not refer to the JSLint project, but to any linting tool for JS (a JS version of the base LintRunner).
It was renamed because it's now being used for both JSHint and JSCS.

Can you suggest a better name?

@ChrisTorng
Copy link
Contributor Author

I know there is #638 to discuss exclusions. I think that thread is for a better conclusion.
For here, the quick fix, Should I change currently empty one?

Code one can't be taken away for we don't have the mechanism to replace existed jscs.json now.
If code one must exist now, should we put the same thing into excludeFiles?

I think no, cause if user try to change them, they will be confused for even they change or delete old one, they just work strange.
But if we left it empty, users will noticed that there are some rules behind, and most should get a conclusion that WE have a default rule. Users can still add new one, but (temporary) can't change the default one.

This default rule is not a good thing, but this can be fix after quick fix version.

So from my point of view, I think minified is a must have (cause they make disaster), been a WE default one. They should stay in hard code right now.

Other common js library, even _referenced.js, can stay in excludeFiles, give user ability to customize them. But still need to consider, currently we don't have mechanism to replace .jscs.json.

Or just change the default file name to .jscsrc this time, and just delete the old one? Current released version's JSCS function cannot use, users almost can only turn it off... Oops, just checked, this still need to wait for jscs's new version...

So should we just replace existed .jscs.json into new one? Will we need to change/add more in later versions? Should we always do auto change on every new version? Or just change this time, later we can still provide better default file, but never affect existed one?

I wanna a quick and easy strategy for this time...

@ChrisTorng
Copy link
Contributor Author

Actually I don't like the idea of one setting controls two tools.
JsHint is for checking potential errors, JSCS is for checking coding style.
There must be some users want to check for errors, but not agree to those coding style.

But I have look into this. Currently, WESetting's LinterSettings have LintOnSave/LintOnBuild, then JavaScriptSettings inherit it. This inheritance make me wonder, and think there may be a reason for combine two tools at one setting.

For a quick fix, I don't think create two new settings for JSCS is a must be. I just wanna JSCS share the same ignore rule with JsHint, then JSCS won't make disaster. So I choose to change name, still control by the same original setting.

Code confusing won't bother users. The better way for users should be separate settings. That means they won't have a common base class, just as TypeScript is a separate class.

@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

For settings, we should have allow the user to choose between running JSCS, JSHint, or both in the WE JS options. (probably either a dropdown list with three entries or two boolean settings)

We would consume this setting when creating lint reporters

@madskristensen
Copy link
Owner

Let me know when this is ready to merge

@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

Unless we can think of a better name for the JsLintReporter class, I think this is ready.

@madskristensen
Copy link
Owner

What about JsLintingReporter?

@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

Good idea

@ChrisTorng
Copy link
Contributor Author

So it becomes class JsLintingReporter : LintReporter ?????????
Should LintReporter becomes LintingReporter???

This may introduce more other changings... Change class name is easy, but I also need to change every parameter shares that name...
And should I change ILintCompiler, LintFileInvoker, LintConfigEncodingFix, TsLintCompiler,... and menu items, settings, ........

I just don't understand what's the influence range of changing Lint into Linting.

I can accept any name we all agreed. But for a quick fix, and for I am not so familiar with this project, better not to introduce lots changes.

I think current name still make sense. I can add a comment to JsLintReporter, to reduce this problem.

For a better name's problem, how about postpone it until current quick fix is released. Then I can start making two tools using two settings? That means from the settings there will be a new name different from LintOnSave/LintOnBuild, then here JsLintReporter may(?) also split into two classes?

@SLaks
Copy link
Collaborator

SLaks commented Feb 9, 2014

Yes; you're right.
How about JavaScriptLintReporter?

@madskristensen
Copy link
Owner

I like it

@madskristensen
Copy link
Owner

Is this ready?

@SLaks
Copy link
Collaborator

SLaks commented Feb 10, 2014

Yes

@SLaks
Copy link
Collaborator

SLaks commented Feb 10, 2014

@ChrisTorng: Instead of making a separate revert commit, you can use git rebase -i to remove the original commit entirely.

madskristensen added a commit that referenced this pull request Feb 10, 2014
Combine JsHint and JSCS rule, hack to stop errors
@madskristensen madskristensen merged commit 4cbb418 into madskristensen:master Feb 10, 2014
ChrisTorng added a commit to ChrisTorng/WebEssentials2013 that referenced this pull request Mar 8, 2014
madskristensen#693 have JSCS 1.3 that fix jscs madskristensen#212, so it's time to undo madskristensen#637 hack to
remove xml item.
madskristensen added a commit that referenced this pull request Mar 8, 2014
JSCS: Undo #637 hack to remove xml item
SLaks pushed a commit to SLaks/WebEssentials2013 that referenced this pull request May 13, 2014
madskristensen#693 have JSCS 1.3 that fix jscs madskristensen#212, so it's time to undo madskristensen#637 hack to
remove xml item.
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.

4 participants