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

No subprocess #112

Merged
Arcanemagus merged 6 commits into from
Jun 13, 2016
Merged

No subprocess #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aa36a3e
Need tiny-promisify for changes
download13 May 17, 2016
903919b
Linter now directly imports htmlhint instead of running it as a
download13 May 17, 2016
3989522
Froze the htmlhint dependency version.
download13 May 17, 2016
f84d299
strip-json-comments is now a dependency
download13 May 17, 2016
f652f2b
Replaced all imports with targeted require calls
download13 May 18, 2016
0755fa9
Removed unneeded config export
download13 May 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 32 additions & 42 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
'use babel';

import * as path from 'path';
import { execNode, find, rangeFromLineNumber } from 'atom-linter';

const GRAMMAR_SCOPES = [
'text.html.angular',
'text.html.basic',
Expand All @@ -14,65 +11,58 @@ const GRAMMAR_SCOPES = [
'text.html.ruby'
];

export const config = {
executablePath: {
title: 'Executable Path',
description: 'HTMLHint Node Script Path',
type: 'string',
default: path.join(__dirname, '..', 'node_modules', 'htmlhint', 'bin', 'htmlhint')
}
};

let executablePath = '';

export function activate() {
require('atom-package-deps').install('linter-htmlhint');
}

executablePath = atom.config.get('linter-htmlhint.executablePath');
function getConfig(filePath) {
const fs = require('fs');
const path = require('path');
const readFile = require('tiny-promisify')(fs.readFile);
const { findAsync } = require('atom-linter');

atom.config.observe('linter-htmlhint.executablePath', newValue => {
executablePath = newValue;
});
return findAsync(path.dirname(filePath), '.htmlhintrc')
.then(configPath => {
Copy link
Member

@Arcanemagus Arcanemagus May 17, 2016

Choose a reason for hiding this comment

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

This might not be a good idea, since it has extra functionality compared to htmlhint itself, but what do you think about using something like cosmiconfig?

Copy link
Contributor Author

@download13 download13 May 18, 2016

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean. Are you concerned about replicating existing htmlhint functionality inside this package? Since htmlhint does not provide an exported getConfig function we're going to need to replicate that functionality in some way.

cosmicconfig could fill this role, but I have two concerns about it. First, it's another module to load which will likely add a few milliseconds to the startup time. Second, it looks for other possible versions of config files as well (.htmlhintrc.yml, .htmlhintrc.js, etc). htmlhint only supports .htmlhintrc. Supporting other forms of the config file might be helpful to someone but strictly speaking it's not how htmlhint behaves and might be confusing to users.

As for the current method, file loading and JSON parsing seem pretty tame as far as adding functionality goes. I'd say we should move it out into another module (or at least file) if there was much more of it, but it's only 18 lines and there's an overhead to using require.

If there's some other package that can replicate the behavior of the original getConfig I'm all for it, but I don't know of one.

Thoughts?

Copy link
Member

@Arcanemagus Arcanemagus May 23, 2016

Choose a reason for hiding this comment

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

I was merely suggesting cosmiconfig as an option for not dealing with loading and parsing the file. Your second point of concern there is what I meant by "extra functionality compared to htmlhint itself.

I'm perfectly fine with how this is implemented now, so let's just go with that 😉

if (configPath) {
return readFile(configPath, 'utf8');
}
return null;
})
.then(conf => {
if (conf) {
return JSON.parse(require('strip-json-comments')(conf));
}
return null;
});
}

export function provideLinter() {
return {
name: 'htmlhint',
grammarScopes: GRAMMAR_SCOPES,
scope: 'file',
lintOnFly: false,
lintOnFly: true,
lint: editor => {
const { HTMLHint } = require('htmlhint');
const text = editor.getText();
const filePath = editor.getPath();

if (!text) {
return Promise.resolve([]);
}

const parameters = [filePath, '--format', 'json'];
const htmlhintrc = find(path.dirname(filePath), '.htmlhintrc');

if (htmlhintrc) {
parameters.push('-c');
parameters.push(htmlhintrc);
}

return execNode(executablePath, parameters, {}).then(output => {
const results = JSON.parse(output);

if (!results.length) {
return [];
}

const messages = results[0].messages;
return getConfig(filePath)
.then(ruleset => HTMLHint.verify(text, ruleset || undefined))
.then(messages => {
const { rangeFromLineNumber } = require('atom-linter');

return messages.map(message => ({
range: rangeFromLineNumber(editor, message.line - 1, message.col - 1),
type: message.type,
text: message.message,
filePath
}));
});
return messages.map(message => ({
range: rangeFromLineNumber(editor, message.line - 1, message.col - 1),
type: message.type,
text: message.message,
filePath
}));
});
}
};
}
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
"dependencies": {
"atom-linter": "^4.6.1",
"atom-package-deps": "^4.0.1",
"htmlhint": "~0.9.12"
"htmlhint": "0.9.12",
"strip-json-comments": "^2.0.1",
"tiny-promisify": "^0.1.1"
},
"devDependencies": {
"eslint": "^2.9.0",
Expand Down