Add rules directory configuration #45

dtminnaar · 2018-09-26T23:38:42Z

We use custom html hinting rules, this is a small improvement to allow you to specify the folder containing those rules.

msftclas · 2018-09-26T23:38:52Z

All CLA requirements met.

aleh-vasilyeu · 2018-09-28T13:58:33Z

@ganralf great job!
@mike-kaufman I look forward to this feature :D

rene-leanix · 2018-11-07T10:17:51Z

@mike-kaufman, would be great if this could be merged some time soon to add support for custom rules.

rene-leanix · 2018-11-07T10:18:52Z

@mike-kaufman, would be great if this could be merged some time soon to add support for custom rules.

mike-kaufman

Thanks @ganralf! I added some questions. Forgive me if any are obvious, as I don't look at this code too frequently. :)

mike-kaufman · 2018-11-07T15:32:29Z

htmlhint-server/src/server.ts

+        rulesLoaded = false;
+    }
+
+    if (rulesLoaded || htmlhint == null) {


what is htmlhint here? The param is HTMLHint.

Correct, that check is pointless

mike-kaufman · 2018-11-07T15:36:21Z

htmlhint-server/src/server.ts

+            loadCustomRules(absoluteDir, HTMLHint);
+        }
+    }
+    catch (e) { }


why are exceptions getting swallowed here? Is this necessary?

loadCustomRules does error handling, I will remove the try/catch in this method

mike-kaufman · 2018-11-07T15:37:30Z

htmlhint-server/src/server.ts

+
+// load custom rles
+function loadCustomRules(rulesdir:string, HTMLHint:any):any{
+    rulesdir = rulesdir.replace(/\\/g, '/');


A comment here would help those of us who don't have a built-in regex parser. :)

Similiarly for other regexes used in here.

I copied both loadCustomRules and loadRule from the htmlhint module's source code as it is not exposed for us to use.

Ideally instead of copy/pasting this, we'd get these methods exposed via htmlhint. This way, we don't break the VS Code exetension when htmlhint changes their implementation. Do you know what a PR would look like to refactor in htmlhint? /cc @thedaviddias.

mike-kaufman · 2018-11-07T15:40:28Z

htmlhint-server/src/server.ts

+        if(fs.statSync(rulesdir).isDirectory()){
+            rulesdir += /\/$/.test(rulesdir)?'':'/';
+            rulesdir += '**/*.js';
+            var arrFiles = glob.sync(rulesdir, {


A comment here explaining what files you're going to load would be helpful as well.

mike-kaufman · 2018-11-07T15:44:01Z

htmlhint-server/src/server.ts

+                'silent': true
+            });
+            arrFiles.forEach(function(file){
+                loadRule(file, HTMLHint);


What's the motivation for letting someone specify a directory here? It seems like doing that will result in the extension require-ing every JS file in the directory, which could easily be problematic.

Can you support a single file, or an explicit list of files?
What does basic htmlhint allow here?

The way the plugin works (as I understand it) is you can create multiple rules in as many files and subfolders as you wish.
Each .js file in the rules folder will be loaded into memory.

I thought they had to be specified in the rules list as well via htmlhintrc or --rules on the command line.

mike-kaufman · 2018-11-07T15:44:32Z

htmlhint-server/src/server.ts

+        var module = require(filepath);
+        module(HTMLHint);
+    }
+    catch(e){}


blanket catch seems problematic here. Is this here for a reaason?

I expect the idea is to skip a rule if it is broken. Given that this code is from htmlhint's source there might not be a way to properly report errors from there.

Can you please suggest a nice way we can report errors to the user as we have access to vscode here?

I think vs code APIs let you report errors with something like connection.window.showErrorMessage(...message...); See API docs [here](https://code.visualstudio.com/docs/extensionAPI/vscode-api_

mike-kaufman · 2018-11-07T15:47:50Z

htmlhint-server/src/server.ts

@@ -60,6 +65,74 @@ function getRange(error: htmlhint.Error, lines: string[]): any {
    };
 }

+function loadRules(HTMLHint: any, force: boolean): any {


for consitency, I'd call HTMLHint linter or htmlHintLinter - something to indicate that it is an instance ofa linter.

Or can you just use the module var linter here?

Yes, that will be better, will make that change

mike-kaufman · 2018-11-07T15:53:50Z

Please @-mention me if I go dark on this. :)

mike-kaufman

some more comments... getting closer :)

mike-kaufman · 2018-11-15T14:08:17Z

htmlhint-server/src/server.ts

+    rulesLoaded = false;
+    if (linter == null || !settings.htmlhint || !settings.htmlhint.rulesDir) {
+		return;
+	}


nit: looks like indentation is messed up in loadRules()

mike-kaufman · 2018-11-15T14:14:59Z

htmlhint-server/src/server.ts

+		return;
+	}
+
+	let rulesDir = settings.htmlhint.rulesDir;


I think you should be able to do something like const rulesDir = path.resolve(settings.htmlhint.rulesDir);, and then drop all the stuff from lines 75-84?

Or alternatively, const rulesDir = path.resolve(rootDir, settings.htmlhint.rulesDir);

mike-kaufman · 2018-11-15T14:19:45Z

htmlhint-server/src/server.ts

+
+// load custom rles
+function loadCustomRules(rulesdir:string, HTMLHint:any):any{
+    rulesdir = rulesdir.replace(/\\/g, '/');


Ideally instead of copy/pasting this, we'd get these methods exposed via htmlhint. This way, we don't break the VS Code exetension when htmlhint changes their implementation. Do you know what a PR would look like to refactor in htmlhint? /cc @thedaviddias.

mike-kaufman · 2018-11-15T14:22:27Z

htmlhint-server/src/server.ts

+        var module = require(filepath);
+        module(HTMLHint);
+    }
+    catch(e){}


I think vs code APIs let you report errors with something like connection.window.showErrorMessage(...message...); See API docs [here](https://code.visualstudio.com/docs/extensionAPI/vscode-api_

miramardesign · 2019-02-08T00:21:50Z

so is there any way to load a /rules directory in .htmlhintrc? is that a different issue?

davehibshman · 2019-02-28T21:57:23Z

@miramardesign I haven't looked at the HTMLHint source, but the documentation suggests custom rules go in a sub-folder and you use the --rulesdir on the command line to pic them up. To turn them on you can either include the custom rule name in your rc file or you can include it on the command line using the --rules option.

carlkenne · 2019-06-13T12:59:54Z

Nice job here guys. What's the status on this one?

konstantintieber · 2019-10-07T15:58:00Z

Do you need help here? This feature would help us tremendously in developing our Angular frontend, by helping users piece together components, that rely on specific contentChildren to work.
For example you have a <dropdown> component, that only works as long as it has <option> children.

Such a linter is little help, when it's only evaluated when executing the npm command, but very helpful, when the IDE evaluates it automatically.

A linter could look like this:

module.exports = (HTMLHint) => {
  HTMLHint.addRule({
    id: 'dropdown',
    description: 'Cannot use dropdown without providing option children',
    init: function (parser, reporter) {
      var self = this;
      var listeningToTagsInDropdown = false;
      var hasOptionChildren = false;
      parser.addListener('tagstart', function(event) {
        var tagName = event.tagName.toLowerCase();
        if (tagName === 'dropdown') {
          listeningToTagsInDropdown = true;
        } else if (tagName === 'option' && listeningToTagsInDropdown) {
            hasOptionChildren = true;
        }
      });
      parser.addListener('tagend', function(event) {
        var tagName = event.tagName.toLowerCase();
        if (tagName === 'dropdown') {
          if (!hasOptionChildren) {
            reporter.warn('Cannot use dropdown without providing option children.', event.line, event.col, self, event.raw);
          }
          listeningToTagsInDropdown = false;
        }
      });
    }
  });
}

andrewbohm · 2020-02-15T00:05:16Z

I too would love to see this feature. Do we know when this pull request will be accepted and merge? Thanks in advance!

mike-kaufman · 2020-02-15T19:06:37Z

there's pending comments on teh PR that haven't been addressed, and conflicts that need resolved. Feel free to open another PR & move this forward. I'm happy to merge things in provided feedback is addressed. :)

Add rules directory configuration

a5e7712

mike-kaufman suggested changes Nov 7, 2018

View reviewed changes

Du Toit added 2 commits November 8, 2018 12:44

Use the module's linter rather than passing HTMLhint as a parameter

cc87359

Update readme and bump version to 0.5.2

bcd52dd

mike-kaufman suggested changes Nov 15, 2018

View reviewed changes

Add rules directory configuration #45

Are you sure you want to change the base?

Add rules directory configuration #45

Conversation

dtminnaar commented Sep 26, 2018

msftclas commented Sep 26, 2018 • edited Loading

aleh-vasilyeu commented Sep 28, 2018 • edited Loading

rene-leanix commented Nov 7, 2018

rene-leanix commented Nov 7, 2018

mike-kaufman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-kaufman commented Nov 7, 2018

mike-kaufman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miramardesign commented Feb 8, 2019

davehibshman commented Feb 28, 2019

carlkenne commented Jun 13, 2019

konstantintieber commented Oct 7, 2019 • edited Loading

andrewbohm commented Feb 15, 2020

mike-kaufman commented Feb 15, 2020

msftclas commented Sep 26, 2018 •

edited

Loading

aleh-vasilyeu commented Sep 28, 2018 •

edited

Loading

konstantintieber commented Oct 7, 2019 •

edited

Loading