Skip to content

Commit

Permalink
Combine JsHint and JSCS rule, hack to stop errors
Browse files Browse the repository at this point in the history 
Combine JsHint and JSCS rule and cleanup excludeFiles. Hack to stop <xml
and more than 500 errors. Revert Regex.
  • Loading branch information
@ChrisTorng
ChrisTorng committed Feb 9, 2014
1 parent 76672b3 commit da5538a
Show file tree
Hide file tree
Showing 9 changed files with 53 additions and 31 deletions.
4 changes: 2 additions & 2 deletions EditorExtensions/Commands/JavaScript/JavaScriptCreationListener.cs
View file
Expand Up @@ -45,12 +45,12 @@ public void VsTextViewCreated(IVsTextView textViewAdapter)
ITextDocument document;
if (TextDocumentFactoryService.TryGetTextDocument(textView.TextDataModel.DocumentBuffer, out document))
{
var jsHintLintInvoker = new LintFileInvoker(f => new JsHintReporter(f), document);
var jsHintLintInvoker = new LintFileInvoker(f => new JsLintReporter(new JsHintCompiler(), f), document);
textView.Closed += (s, e) => jsHintLintInvoker.Dispose();

textView.TextBuffer.Properties.GetOrCreateSingletonProperty(() => jsHintLintInvoker);

var jsCodeStyleLintInvoker = new LintFileInvoker(f => new LintReporter(new JsCodeStyleCompiler(), WESettings.Instance.JavaScript, f), document);
var jsCodeStyleLintInvoker = new LintFileInvoker(f => new JsLintReporter(new JsCodeStyleCompiler(), f), document);
textView.Closed += (s, e) => jsCodeStyleLintInvoker.Dispose();

textView.TextBuffer.Properties.GetOrCreateSingletonProperty(() => jsCodeStyleLintInvoker);
Expand Down
33 changes: 17 additions & 16 deletions ...ons/Commands/JavaScript/JsHintReporter.cs → ...ons/Commands/JavaScript/JsLintReporter.cs
View file
Expand Up @@ -6,9 +6,10 @@

namespace MadsKristensen.EditorExtensions
{
internal class JsHintReporter : LintReporter
internal class JsLintReporter : LintReporter
{
public JsHintReporter(string fileName) : base(new JsHintCompiler(), WESettings.Instance.JavaScript, fileName) { }
public JsLintReporter(ILintCompiler lintCompiler, string fileName) :
base(lintCompiler, WESettings.Instance.JavaScript, fileName) { }

public override Task RunLinterAsync()
{
Expand All @@ -21,7 +22,7 @@ public override Task RunLinterAsync()
return base.RunLinterAsync();
}

public static bool NotJsOrIsMinifiedOrNotExists(string file)
public static bool NotJsOrMinifiedOrDocumentOrNotExists(string file)
{
return !Path.GetExtension(file).Equals(".js", StringComparison.OrdinalIgnoreCase) ||

This comment has been minimized.

Sign in to view

Copy link
@SLaks

SLaks Feb 9, 2014

Actually, we can put all of this logic into the regex.
file.EndsWith(".min.js", StringComparison.OrdinalIgnoreCase) ||
Expand All @@ -33,20 +34,20 @@ public static bool NotJsOrIsMinifiedOrNotExists(string file)

public static bool ShouldIgnore(string file)
{
if (NotJsOrIsMinifiedOrNotExists(file))
if (NotJsOrMinifiedOrDocumentOrNotExists(file))
{
return true;
}

ProjectItem item = ProjectHelpers.GetProjectItem(file);

if (item == null)
return true;

// Ignore files nested under other files such as bundle or TypeScript output
ProjectItem parent = item.Collection.Parent as ProjectItem;
if (parent != null && !Directory.Exists(parent.FileNames[1]) || File.Exists(item.FileNames[1] + ".bundle"))
return true;
if (item != null)
{
// Ignore files nested under other files such as bundle or TypeScript output
ProjectItem parent = item.Collection.Parent as ProjectItem;
if (parent != null && !Directory.Exists(parent.FileNames[1]) || File.Exists(item.FileNames[1] + ".bundle"))
return true;
}

string name = Path.GetFileName(file);
return _builtInIgnoreRegex.IsMatch(name);
Expand All @@ -61,19 +62,19 @@ public static bool ShouldIgnore(string file)
@"dojo\.js",
@"ember\.js",
@"ext-core\.js",
@"handlebars\.*",
@"handlebars.*",
@"highlight\.js",
@"history\.js",
@"jquery-([0-9\.]+)\.js",
@"jquery\.blockui\.*",
@"jquery\.validate\.*",
@"jquery\.unobtrusive\.*",
@"jquery.blockui.*",
@"jquery.validate.*",
@"jquery.unobtrusive.*",
@"jquery-ui-([0-9\.]+)\.js",
@"json2\.js",
@"knockout-([0-9\.]+)\.js",
@"MicrosoftAjax([a-z]+)\.js",
@"modernizr-([0-9\.]+)\.js",
@"mustache\.*",
@"mustache.*",
@"prototype\.js ",
@"qunit-([0-9a-z\.]+)\.js",
@"require\.js",
Expand Down
24 changes: 21 additions & 3 deletions EditorExtensions/Commands/LintReporter.cs
View file
Expand Up @@ -103,9 +103,27 @@ private void ReadResult(IEnumerable<CompilerError> results)
_provider.Tasks.Remove(task);
}

// HACK: stop JSCS (and any other) from output more than 500 items, hope JSCS can do this
int errorLimit = 500;
int resultCount = results.Count();
int count = 0;
foreach (CompilerError error in results.Where(r => r != null))
{
ErrorTask task = CreateTask(error);
ErrorTask task;

// JSHint has build in limit up to 500
// the 501 one is "too many" one, no need to replace it with this hack
if (++count > errorLimit &&
!(_compiler is JsHintCompiler && resultCount == 501))
{
task = CreateTask(error,
string.Format("{0}: Too many errors. Only shows {1} errors from {2}.",
_compiler.ServiceName, errorLimit, results.Count()));
_provider.Tasks.Add(task);
break;
}

task = CreateTask(error);
_provider.Tasks.Add(task);
}
}
Expand All @@ -116,7 +134,7 @@ private void ReadResult(IEnumerable<CompilerError> results)
}
}

private ErrorTask CreateTask(CompilerError error)
private ErrorTask CreateTask(CompilerError error, string message = null)
{
ErrorTask task = new ErrorTask()
{
Expand All @@ -126,7 +144,7 @@ private ErrorTask CreateTask(CompilerError error)
Category = TaskCategory.Html,
Document = error.FileName,
Priority = TaskPriority.Low,
Text = error.Message,
Text = message ?? error.Message,
};

task.AddHierarchyItem();
Expand Down
9 changes: 6 additions & 3 deletions EditorExtensions/Compilers/NodeExecutorBase.cs
View file
Expand Up @@ -99,16 +99,19 @@ private CompilerResult ProcessResult(Process process, string errorText, string s
private void ValidateResult(Process process, string outputFile, string errorText, CompilerResult result)
{
try
{ /* Temporary hack see; //github.com/mdevils/node-jscs/issues/212 */
if (process.ExitCode == 0 && !errorText.StartsWith("<?xml version=", StringComparison.CurrentCulture))
{
if (process.ExitCode == 0 &&
/* Temporary hack see; //github.com/mdevils/node-jscs/issues/212 */
(!errorText.StartsWith("<?xml version=", StringComparison.CurrentCulture) ||
errorText == "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<checkstyle version=\"4.3\">\n</checkstyle>"))
{
if (!string.IsNullOrEmpty(outputFile))
result.Result = File.ReadAllText(outputFile);
result.IsSuccess = true;
}
else
{
result.Errors = ParseErrors(errorText.Replace("\r", ""));
result.Errors = ParseErrors(errorText.Replace("\n", ""));

This comment has been minimized.

Sign in to view

Copy link
@SLaks

SLaks Feb 9, 2014

@am11: Why do we care about character count in the XML?

This comment has been minimized.

Sign in to view

Copy link
@SLaks

SLaks Feb 9, 2014

Yes, but why do we care about \r at all?

This comment has been minimized.

Sign in to view

Copy link
@ChrisTorng

ChrisTorng Feb 9, 2014

Author Owner

Sorry for I don't really understand '\r' for. So, Revert it to '\n' or remove .Replace()?
But for Error List, It is not using fixed pitch by default, so maybe it will only misleading user. But flattern is even more misleading, right. We can only read '^' correctly from Output window.

This comment has been minimized.

Sign in to view

Copy link
@SLaks

SLaks Feb 9, 2014

You're right; ^-style errors won't work well in the error list. However, removing the newline entirely will just make it worse; I think we should remove the Replace() call entirely.

As a more ideal, complicated solution, we should try to remove that text (code line & ^ line) from error messages. Since you can double-click the error to jump to that location in the editor, we don't need to display the source line in the error.
Doing that will probably require supporting changes in JSCS.
}
}
catch (FileNotFoundException missingFileException)
Expand Down
4 changes: 2 additions & 2 deletions EditorExtensions/EditorExtensionsPackage.cs
View file
Expand Up @@ -155,9 +155,9 @@ private void BuildEvents_OnBuildDone(vsBuildScope Scope, vsBuildAction Action)

if (WESettings.Instance.JavaScript.LintOnBuild)
{
LintFileInvoker.RunOnAllFilesInProjectAsync(".js", f => new JsHintReporter(f))
LintFileInvoker.RunOnAllFilesInProjectAsync(".js", f => new JsLintReporter(new JsHintCompiler(), f))
.DontWait("running solution-wide JSHint");
LintFileInvoker.RunOnAllFilesInProjectAsync(".js", f => new LintReporter(new JsCodeStyleCompiler(), WESettings.Instance.JavaScript, f))
LintFileInvoker.RunOnAllFilesInProjectAsync(".js", f => new JsLintReporter(new JsCodeStyleCompiler(), f))
.DontWait("running solution-wide JSCS");
}

Expand Down
2 changes: 1 addition & 1 deletion EditorExtensions/MenuItems/JsCodeStyle.cs
View file
Expand Up @@ -45,7 +45,7 @@ void menuCommand_BeforeQueryStatus(object sender, System.EventArgs e)
OleMenuCommand menuCommand = sender as OleMenuCommand;

files = ProjectHelpers.GetSelectedFilePaths()
.Where(f => !JsHintReporter.NotJsOrIsMinifiedOrNotExists(f)).ToList();
.Where(f => !JsLintReporter.NotJsOrMinifiedOrDocumentOrNotExists(f)).ToList();

menuCommand.Enabled = files.Count > 0;
}
Expand Down
4 changes: 2 additions & 2 deletions EditorExtensions/MenuItems/JsHint.cs
View file
Expand Up @@ -44,7 +44,7 @@ void menuCommand_BeforeQueryStatus(object sender, System.EventArgs e)
OleMenuCommand menuCommand = sender as OleMenuCommand;

var raw = ProjectHelpers.GetSelectedFilePaths();
files = raw.Where(f => !JsHintReporter.NotJsOrIsMinifiedOrNotExists(f)).ToList();
files = raw.Where(f => !JsLintReporter.NotJsOrMinifiedOrDocumentOrNotExists(f)).ToList();

menuCommand.Enabled = files.Count > 0;
}
Expand All @@ -56,7 +56,7 @@ private void RunJsHint()

foreach (string file in files)
{
JsHintReporter runner = new JsHintReporter(file);
JsLintReporter runner = new JsLintReporter(new JsHintCompiler(), file);
runner.RunLinterAsync().DontWait("linting " + file);
}
}
Expand Down
2 changes: 1 addition & 1 deletion EditorExtensions/Resources/settings-defaults/.jscs.json
View file
Expand Up @@ -9,7 +9,7 @@
"disallowKeywords": ["with"],
"disallowMultipleLineBreaks": true,
"disallowKeywordsOnNewLine": ["else"],
"excludeFiles": ["**/*.min.js", "**/*.debug.js", "**/*.intellisense.js", "**/*-vsdoc.js"],
"excludeFiles": [],

This comment has been minimized.

Sign in to view

Copy link
@SLaks

SLaks Feb 9, 2014

Since we now exclude these files in the Reporter, there is no reason to exclude them again in config.
(unless the user wants to get the same results by invoking JSCS on the command-line)

This comment has been minimized.

Sign in to view

Copy link
@SLaks

SLaks Feb 9, 2014

@am11: That is a debatable question.
In principle, it's certainly better to use config files for interoperability with non-VS developers.

However, config files cannot exclude compiled TS/CoffeeScript results, files excluded from project, etc.
Also, having global default exclusions so that people who don't set up config files aren't deluged with errors (see the dozen GH issues from 1.8) is trickier.

I've been thinking about opening an issue to discuss this question; I'm not sure if there is a good answer.

This comment has been minimized.

Sign in to view

Copy link
@SLaks

SLaks Feb 9, 2014

See madskristensen#638
"validateJSDoc": {
"checkParamNames": true,
"requireParamTypes": true
Expand Down
2 changes: 1 addition & 1 deletion EditorExtensions/WebEssentials2013.csproj
View file
Expand Up @@ -437,7 +437,7 @@
<Compile Include="Commands\JavaScript\JavaScriptFindReferencesCommandTarget.cs">
<SubType>Code</SubType>
</Compile>
<Compile Include="Commands\JavaScript\JsHintReporter.cs">
<Compile Include="Commands\JavaScript\JsLintReporter.cs">
<SubType>Code</SubType>
</Compile>
<Compile Include="Commands\LESS\LessExtractMixinCommandTarget.cs" />
Expand Down

3 comments on commit da5538a

@ChrisTorng
Copy link
Owner Author

Choose a reason for hiding this comment

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

What I do these days, is trying to do some easy & fast fix, to stop all the problem starting from WE 1.8.

For this PR, I did:

Combine JSHint and JSCS ignore together, so make JsHintReporter into JsLintReporter, add a new parameter compiler, so each time wanna run JSHint or JSCS should use this class, that share the same rule. And while item is not referenced by project, it will not always ignore anymore. User can right click on not referenced js, or File - Open - File or drag, still to see the result of JSHint and JSCS.

For why remove pattern from excludeFiles, I'm thinking about another problem.
New version of WE won't replace the existed config file, right? That means even we push a new version .jscs.json with correct excludeFiles, those pre-existed one will still keep the original "test/data/*.js" one. For make this work done, the easiest and fast way is do it from code. And I think minified/intellisense js will never have any meaning to be Lint. Why should they always keep excludeFiles with these nonsense?

For hack to remove xml item, of cause we should wait for CLI: Incorrect ERRORLEVEL being reported to stdOut #212, but it's over 2/7, when will it release new version? This issue is not urgent to them, but really annoying to WE users. I can remove that hack after final fix is done.

For 500 limit, I just open a new issue Eliminate total error count #238. But are they really consider to accept? Is it high priority? When will they release? But now WE users are facing serious performance problem, generate from opening js for thousands items.

Thinking about all WE 1.8x users, some can figure out to turn "Run on build/save" off. But there are many choose to downgrade, even to disable/uninstall. Maybe some just don't understand why their VS hang on every build, just trying to reinstall VS again. How long should we all wait for JSCS?

All my goal is to have a quick fix as fast as possible. Mainly just stop JSCS to run on minified js, ignore xml item, 500 limit for other big js files. With all these changes, I think we can again use "Run on build/save" happily.

Really hope to have a good enough WE again, for all users. We can push final fix later, but that will not be urgent, cause we already have a good one right now.

@ChrisTorng
Copy link
Owner Author

Choose a reason for hiding this comment

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

For this is my first time to participate to an open source project. Forgive me if break the common rule.

If anyone find a mistake from other's code, should he notify the author to change, or just fix it? Or do you have a private task list, a distributer, everyone will get some tasks to work? Or you have a commander (@madskristensen?) to give you orders?

And this time, WE facing serious problem. Did you all talks before, all agreed to wait for new version of JSCS then start fix? For me, I think that we should not wait for others, we can do some quick hack to stop the mess, push new version as fast as possible. Or I should start a new issue, make sure you all agreed a quick fix is necessary, then I can do it?

Again, forgive me for my first time.

@SLaks
Copy link

@SLaks SLaks commented on da5538a Feb 9, 2014

Choose a reason for hiding this comment

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

Again, thank you for your contribution; this is a change that we really need (pending a better way to handle ignores).

You haven't done anything particularly wrong; it's just that we still don't have a perfect solution to this issue.

And yes; we can probably use better coordination. :)

Please sign in to comment.