Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Wrong path to htmlhint #69

Merged
merged 2 commits into from
Jan 22, 2016
Merged

Wrong path to htmlhint #69

merged 2 commits into from
Jan 22, 2016

Conversation

AnWeber
Copy link
Contributor

@AnWeber AnWeber commented Jan 22, 2016

the issue #65 is produced to the wrong path to the htmlhint file. My previous fix attempt by changing to exec instead of execNode was just a workaround. I didn't notice the wrong path.

the issue #65 is produced to the wrong path to the htmlhint file. My previous fix attempt by changing to exec instead of execNode was just a workaround. I didn't notice the wrong path.
@johnwebbcole
Copy link
Contributor

The .bin directory should be there... What OS are you on? It seems APM moves executables to a common directory.

@Arcanemagus
Copy link
Member

apm and npm both should be creating that path, the issue was simply exec vs execNode as you originally surmised.

What version of Atom are you using?

@Arcanemagus
Copy link
Member

Just looked over the rest of the comments in #65, and then checked on the htmlhint "executable" scripts in this folder. They assume you have node installed, which isn't necessarily valid for all Atom users.

So although this is the "wrong" path for executing the htmlhint "binary", it's the right path for our case. Before this gets merged @AnWeber can you update the description of the setting to note that this is the node script, not the executable?

Something like: "HTMLHint Node executable" is probably enough to hint as to which one that should be chosen.

@AnWeber
Copy link
Contributor Author

AnWeber commented Jan 22, 2016

I am using Windows 10 and Atom 1.4. NodeJS was installed while installing the plugin. Afterwards I uninstalled NodeJS to test the issue without NodeJS installed.
@Arcanemagus the new path is also the executable. The script would bin in htmlhint/lib/htmlhint.js.

@Arcanemagus
Copy link
Member

@AnWeber:

// This is the wrapper script ("system binary") which requires node to be installed
path.join(__dirname, '..', 'node_modules', '.bin', 'htmlhint');

// This is the node executable script, which Atom can run and the above are wrappers around
path.join(__dirname, '..', 'node_modules', 'htmlhint', 'bin', 'htmlhint');

// This is the (minified) htmlhint script itself, and is not executable unless ran through something
path.join(__dirname, '..', 'node_modules', 'htmlhint', 'lib', 'htmlhint.js');

@AnWeber
Copy link
Contributor Author

AnWeber commented Jan 22, 2016

thanks for clarification. I changed the name of the setting to 'HTMLHint Node Script Path'

@Arcanemagus
Copy link
Member

Push it up, and assuming @johnwebbcole has no comments we can get it merged 😉.

@AnWeber
Copy link
Contributor Author

AnWeber commented Jan 22, 2016

Ready. I forgot to push

@johnwebbcole
Copy link
Contributor

It's ok with me... The original script used the bin, not the .bin. I like the .bin idea, but it doesn't look like it's valid on all platforms

@Arcanemagus
Copy link
Member

Looks like Atom didn't get added to the PATH in the AppVeyor build for some reason, pinged @steelbrain to rebuild that then we can merge.

Arcanemagus added a commit that referenced this pull request Jan 22, 2016
@Arcanemagus Arcanemagus merged commit 0902105 into AtomLinter:master Jan 22, 2016
@AnWeber AnWeber deleted the patch-1 branch January 22, 2016 22:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants