add rulesdir option #7

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

Jump to bottom

Merged

mattlewis92 merged 11 commits into htmlhint:master from Benoit-Vasseur:master

Apr 25, 2017

Merged

add rulesdir option #7

mattlewis92 merged 11 commits into htmlhint:master from Benoit-Vasseur:master

Apr 25, 2017

Conversation

@Benoit-Vasseur

Copy link

Contributor

@Benoit-Vasseur Benoit-Vasseur commented Apr 20, 2017

Hi,

We are curently using VueJs and to be able to have your loader working we had to use the rulesdir feature of HtmlHint.
We think this could be useful for anyone.
The code is inspired by HtmlHint itself : https://github.com/yaniswang/HTMLHint/blob/152a114f277645474ca1f70d68a0e01e75f3eb51/bin/htmlhint#L142 .

What do you think ?

Benoit-Vasseur added 3 commits

April 20, 2017 10:14

@Benoit-Vasseur


 add rulesdir option

beb55fb

@Benoit-Vasseur


 pass error to emitter instead of string to have cleaner error report ...

5278e22

...(friendly-errors-webpack-plugin)

@Benoit-Vasseur


 fix style rules

9e8d4f2

@Benoit-Vasseur

Copy link

Contributor Author

Benoit-Vasseur commented Apr 20, 2017

Dont't understand why the tests are failing :/ They are green on my machine

mattlewis92

mattlewis92 requested changes

Apr 20, 2017

View reviewed changes

Copy link

Member

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks! Could you also add a test for the new feature + document its usage in the readme.

index.js Outdated

}

// load custom rles

function loadCustomRules(rulesdir) {

Copy link

Member

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

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

Could this function be replaced with a call to node-glob?

index.js Outdated

/* eslint-disable import/no-dynamic-require */ // --> OFF

filepath = path.resolve(filepath);

try {

var module = require(filepath);

Copy link

Member

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

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

Please change this line to const module = require(filepath); // eslint-disable-line import/no-dynamic-require. I also would use a different variable instead of module to prevent shadowing the global module variable. Maybe rule instead?

index.js Outdated

if (options.customRules) {

options.customRules.forEach(rule => HTMLHint.addRule(rule));

}

if (options.rulesdir) {

Copy link

Member

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

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

Can you camelcase this option to rulesDir instead?

index.js

}

emitter(options.formatter(report));

emitter(newError(options.formatter(report)));

Copy link

Member

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

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

I believe the tests are failing because this is now being emitted as an error. This shouldn't be the case, especially if emitting as a warning not an error.

Copy link

Contributor Author

@Benoit-Vasseur Benoit-Vasseur Apr 21, 2017

Choose a reason for hiding this comment

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

On my dev machine all the tests are green. And I added this because of a warning in friendly-errors-webpack-plugin : "value string instead of error instance".
I do not understand why they are failing on Travis :/

Copy link

Member

@mattlewis92 mattlewis92 Apr 21, 2017

Choose a reason for hiding this comment

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

aha, I realised what was going wrong, travis + my local dev envrionment uses the yarn.lock which has an older version of webpack in the dependencies. It seems like webpack introduced a breaking change between 2.2.1 and 2.4.1 that throws if you emit non errors. If you sync your branch with master the tests should start passing 😄

@Benoit-Vasseur

Copy link

Contributor Author

Benoit-Vasseur commented Apr 20, 2017

Thanks for the review. Il will try to do the changes tomorrow.

@Benoit-Vasseur


 add test + modify readme + use glob

70cbf17

@Benoit-Vasseur

Copy link

Contributor Author

Benoit-Vasseur commented Apr 21, 2017

I made the changes.
However the tests are failing on Travis :/ I do not understand why (it works on my machine). Could you look at it ? Thanks.

@mattlewis92


 fix minor typo

827d434

mattlewis92

mattlewis92 requested changes

Apr 22, 2017

View reviewed changes

index.js Outdated

const ruleOption = options[ruleObj.id]; // we can pass a value to the rule

ruleObj.rule(HTMLHint, ruleOption);

} catch (err) {

throw new Error(err.message);

Copy link

Member

@mattlewis92 mattlewis92 Apr 22, 2017

Choose a reason for hiding this comment

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

Is this try catch necessary? By wrapping it like this you'll lose the stack trace etc

package.json

"chai": "^3.5.0",

"codecov-lite": "^0.1.3",

"glob": "^7.1.1",

"mocha": "^3.0.2",

Copy link

Member

@mattlewis92 mattlewis92 Apr 22, 2017

Choose a reason for hiding this comment

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

This should be a dependency, not a dev dependency (also make sure the yarn.lock file is updated)

mattlewis92 and others added 6 commits

April 24, 2017 11:57

@mattlewis92


 Update readme to reflect webpack 2 docs changes that was recently merged

b24dac1


 Merge remote-tracking branch 'source/master'

fd52956


 remove try / catch in "loadRule"

a92c8d3


 move "glob" package to dependencies

c778d10


 update yarn.lock file

e926ba1

@mattlewis92


 Fix tests on node 4

03c185a

@codecov-io

Copy link

codecov-io commented Apr 25, 2017 •

edited

Loading

Codecov Report

Merging #7 into master will increase coverage by 1.09%.
The diff coverage is 93.75%.

Impacted file tree graph

@@ Coverage Diff @@
## master #7 +/- ##
==========================================
+ Coverage 88.05% 89.15% +1.09% 
==========================================
 Files 1 1 
 Lines 67 83 +16 
==========================================
+ Hits 59 74 +15 
- Misses 8 9 +1

Impacted Files	Coverage Δ
index.js	`89.15% <93.75%> (+1.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edbe992...03c185a. Read the comment docs.

mattlewis92

mattlewis92 approved these changes

Apr 25, 2017

View reviewed changes

@mattlewis92 mattlewis92 merged commit 4f33df4 into htmlhint:master

Apr 25, 2017

@mattlewis92

Copy link

Member

mattlewis92 commented Apr 25, 2017

Released as 1.2.0, thanks for the contribution! 💯

@Benoit-Vasseur

Copy link

Contributor Author

Benoit-Vasseur commented Apr 25, 2017

Great :)
Thanks for your help :)

Labels

None yet

3 participants

@Benoit-Vasseur @codecov-io @mattlewis92

Uh oh!

add rulesdir option #7

add rulesdir option #7

Uh oh!

Conversation

@Benoit-Vasseur Benoit-Vasseur commented Apr 20, 2017

Uh oh!

Benoit-Vasseur commented Apr 20, 2017

Uh oh!

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

Uh oh!

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

@mattlewis92 mattlewis92 Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

@Benoit-Vasseur Benoit-Vasseur Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

@mattlewis92 mattlewis92 Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

Benoit-Vasseur commented Apr 20, 2017

Uh oh!

Benoit-Vasseur commented Apr 21, 2017

Uh oh!

@mattlewis92 mattlewis92 Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

@mattlewis92 mattlewis92 Apr 22, 2017

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattlewis92 commented Apr 25, 2017

Uh oh!

Benoit-Vasseur commented Apr 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Apr 25, 2017 •

edited

Loading