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 #163 #181

Closed
wants to merge 3 commits into from
Closed

Fix #163 #181

wants to merge 3 commits into from

Conversation

phaux
Copy link

@phaux phaux commented Nov 16, 2016

Make htmlhint check the root for the config file. Fixes #163

Same issue here. I added a few console.logs and it seems that it quits the loop one iteration too soon and skips checking the root.

So when doing htmlhint foo/bar/baz/**/*.html it first figures out the root of the glob which is foo/bar/baz and checks foo/bar/baz, foo/bar and foo. It never checks empty string path

@coveralls
Copy link

coveralls commented Nov 16, 2016

Coverage Status

Coverage remained the same at 98.734% when pulling 72fd710 on phaux:patch-1 into 152a114 on yaniswang:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.734% when pulling 72fd710 on phaux:patch-1 into 152a114 on yaniswang:master.

@trisys3
Copy link

trisys3 commented Dec 29, 2016

Is this going to get merged anytime soon?

@phaux
Copy link
Author

phaux commented Jan 3, 2017

My current workaround is to just add -c .htmlhintrc to the command, so instead that

{
  "scripts": {
    "test": "htmlhint src/**/*.html"
  }
}

I've got

{
  "scripts": {
    "test": "htmlhint -c .htmlhintrc src/**/*.html"
  }
}

@jonsmithers
Copy link

I imagine another workaround could be this:

npm remove htmlhint
npm install "https://github.com/phaux/HTMLHint.git#patch-1"

@trisys3
Copy link

trisys3 commented May 1, 2017

Are you hinting that @phaux has a patch for this and he will merge it into master soon?

@jonsmithers
Copy link

@trisys3 I mean....this Pull Request is the patch, but, as you know, it's taking a while to get merged.
I'm just saying that instead of installing the last official release (which npm install htmlhint does), we can install @phaux's patched version by referencing this branch directly (npm install "https://github.com/phaux/HTMLHint.git#patch-1").

@trisys3
Copy link

trisys3 commented May 1, 2017

Oh, I don't know how GitHub patch URL's work, I'll have to look them up later.

@thedaviddias thedaviddias self-requested a review August 22, 2018 20:11
@thedaviddias thedaviddias self-assigned this Aug 22, 2018
@@ -300,12 +300,13 @@ function getConfig(configPath, base, formatter){
if(fs.statSync(base).isDirectory() === false){
base = path.dirname(base);
}
while(base){
var tmpConfigFile = path.resolve(base+path.sep, '.htmlhintrc');
while(true){
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, maybe that works
But can you rewrite this in a way that does not use an always true statement in the while clause?

This conflicts with the eslint rule no-constant-condition and we want that later in our project

@codecov-io
Copy link

Codecov Report

Merging #181 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #181   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           1      1           
=====================================
  Hits            1      1

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 4115caa...a608632. Read the comment docs.

@thedaviddias thedaviddias assigned phaux and unassigned thedaviddias Aug 26, 2018
@thedaviddias thedaviddias added #status: awaiting reply enhancement Functionality that enhances existing features labels Aug 26, 2018
thedaviddias added a commit that referenced this pull request Sep 1, 2018
thedaviddias added a commit that referenced this pull request Sep 1, 2018
**Fixes**: #163 and #181

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:

The fix was originally suggested by @phaux and make HTMLHint checking the root for the config file. 

#### Proposed changes:

- Change the tmpConfigFilve variable.
- Force the console to "break" if base is not found.
thedaviddias added a commit that referenced this pull request Sep 3, 2018
**Fixes**: #163 and #181

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:

The fix was originally suggested by @phaux and make HTMLHint checking the root for the config file. 

#### Proposed changes:

- Change the tmpConfigFilve variable.
- Force the console to "break" if base is not found.
thedaviddias added a commit that referenced this pull request Sep 3, 2018
**Fixes**: #163 and #181

- [x] Check the commit's or even all commits' message styles matches our requested structure.
- [x] Check your code additions will fail neither code linting checks nor unit test.

#### Short description of what this resolves:

The fix was originally suggested by @phaux and make HTMLHint checking the root for the config file. 

#### Proposed changes:

- Change the tmpConfigFilve variable.
- Force the console to "break" if base is not found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants