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 4 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
64 changes: 27 additions & 37 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
'use babel';

import * as fs from 'fs';
Copy link
Member

@Arcanemagus Arcanemagus May 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

If possible these modules should be lazily loaded, I've used lazy-req in the past, but if you have a better method go for it 😉.

Right now this will increase the package loading time as these will be required on Atom launch.

(This comment goes for all the imports.)

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 changed out all the imports for require calls in the places where they are needed. It looks a little odd, but it shaved about 20-30ms off the load time.

import * as path from 'path';
import { execNode, find, rangeFromLineNumber } from 'atom-linter';
import { findAsync, rangeFromLineNumber } from 'atom-linter';
import stripJsonComments from 'strip-json-comments';
import { HTMLHint } from 'htmlhint';
import promisify from 'tiny-promisify';

const readFile = promisify(fs.readFile);
const GRAMMAR_SCOPES = [
'text.html.angular',
'text.html.basic',
Expand All @@ -14,33 +19,34 @@ 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 const config = {};
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.

No need for this line.


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

executablePath = atom.config.get('linter-htmlhint.executablePath');

atom.config.observe('linter-htmlhint.executablePath', newValue => {
executablePath = newValue;
});
function getConfig(filePath) {
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(stripJsonComments(conf));
}
return null;
});
}

export function provideLinter() {
return {
name: 'htmlhint',
grammarScopes: GRAMMAR_SCOPES,
scope: 'file',
lintOnFly: false,
lintOnFly: true,
lint: editor => {
const text = editor.getText();
const filePath = editor.getPath();
Expand All @@ -49,30 +55,14 @@ export function provideLinter() {
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 messages.map(message => ({
return getConfig(filePath)
.then(ruleset => HTMLHint.verify(text, ruleset || undefined))
.then(messages => 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