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

Add support for endLine and endColumn #709

Merged
merged 3 commits into from
Oct 10, 2016
Merged

Add support for endLine and endColumn #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70830c0
Add support for endLine and endColumn
Arcanemagus Sep 16, 2016
8b42b29
Handle types a bit better
Arcanemagus Oct 10, 2016
192925d
Fix lint issue
Arcanemagus Oct 10, 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
13 changes: 11 additions & 2 deletions lib/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Object.defineProperty(exports, "__esModule", {
exports.spawnWorker = spawnWorker;
exports.showError = showError;
exports.idsToIgnoredRules = idsToIgnoredRules;
exports.validatePoint = validatePoint;

var _child_process = require('child_process');

Expand Down Expand Up @@ -49,7 +50,7 @@ function spawnWorker() {
}

function showError(givenMessage) {
var givenDetail = arguments.length <= 1 || arguments[1] === undefined ? null : arguments[1];
var givenDetail = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : null;

var detail = void 0;
var message = void 0;
Expand All @@ -67,10 +68,18 @@ function showError(givenMessage) {
}

function idsToIgnoredRules() {
var ruleIds = arguments.length <= 0 || arguments[0] === undefined ? [] : arguments[0];
var ruleIds = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : [];

return ruleIds.reduce(function (ids, id) {
ids[id] = RULE_OFF_SEVERITY;
return ids;
}, {});
}

function validatePoint(textEditor, line, col) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be validateRange and do similar logic but just use TextBuffer::clipRange? It would save calling it twice...

var buffer = textEditor.getBuffer();
// Clip the given point to a valid one, and check if it equals the original
if (!buffer.clipPosition([line, col]).isEqual([line, col])) {
throw new Error(line + ':' + col + ' isn\'t a valid point!');
}
}
21 changes: 19 additions & 2 deletions lib/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ module.exports = {
'linter-eslint:debug': function linterEslintDebug() {
var textEditor = atom.workspace.getActiveTextEditor();
var filePath = textEditor.getPath();
// eslint-disable-next-line import/no-dynamic-require
var linterEslintMeta = require(_path2.default.join(atom.packages.resolvePackagePath('linter-eslint'), 'package.json'));
var config = atom.config.get('linter-eslint');
var configString = JSON.stringify(config, null, 2);
Expand All @@ -84,7 +85,9 @@ module.exports = {
config: config,
filePath: filePath
}).then(function (response) {
var detail = ['atom version: ' + atom.getVersion(), 'linter-eslint version: ' + linterEslintMeta.version, 'eslint version: ' + require(_path2.default.join(response.path, 'package.json')).version, 'hours since last atom restart: ' + Math.round(hoursSinceRestart * 10) / 10, 'platform: ' + process.platform, 'Using ' + response.type + ' eslint from ' + response.path, 'linter-eslint configuration: ' + configString].join('\n');
var detail = ['atom version: ' + atom.getVersion(), 'linter-eslint version: ' + linterEslintMeta.version,
// eslint-disable-next-line import/no-dynamic-require
'eslint version: ' + require(_path2.default.join(response.path, 'package.json')).version, 'hours since last atom restart: ' + Math.round(hoursSinceRestart * 10) / 10, 'platform: ' + process.platform, 'Using ' + response.type + ' eslint from ' + response.path, 'linter-eslint configuration: ' + configString].join('\n');
var notificationOptions = { detail: detail, dismissable: true };
atom.notifications.addInfo('linter-eslint debugging information', notificationOptions);
}).catch(function (response) {
Expand Down Expand Up @@ -193,6 +196,8 @@ module.exports = {
var ruleId = _ref.ruleId;
var column = _ref.column;
var fix = _ref.fix;
var endLine = _ref.endLine;
var endColumn = _ref.endColumn;

var textBuffer = textEditor.getBuffer();
var linterFix = null;
Expand All @@ -204,8 +209,20 @@ module.exports = {
};
}
var range = void 0;
var msgLine = line - 1;
try {
range = Helpers.rangeFromLineNumber(textEditor, line - 1, column ? column - 1 : column);
if (typeof endColumn !== 'undefined' && typeof endLine !== 'undefined') {
// Here we always want the column to be a number
var msgCol = Math.max(0, column - 1);
(0, _helpers.validatePoint)(textEditor, msgLine, msgCol);
(0, _helpers.validatePoint)(textEditor, endLine - 1, endColumn - 1);
range = [[msgLine, msgCol], [endLine - 1, endColumn - 1]];
} else {
// We want msgCol to remain undefined if it was initially so
// `rangeFromLineNumber` will give us a range over the entire line
var _msgCol = typeof column !== 'undefined' ? column - 1 : column;
range = Helpers.rangeFromLineNumber(textEditor, msgLine, _msgCol);
}
} catch (err) {
throw new Error('Cannot mark location in editor for (' + ruleId + ') - (' + message + ')' + (' at line (' + line + ') column (' + column + ')'));
}
Expand Down
6 changes: 6 additions & 0 deletions spec/fixtures/end-range/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
root: true,
rules: {
'no-unreachable': 'error'
}
};
8 changes: 8 additions & 0 deletions spec/fixtures/end-range/no-unreachable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

function foo() {
return 42;

var a = 4;
return a + 2;
}
41 changes: 30 additions & 11 deletions spec/linter-eslint-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,23 @@ import { tmpdir } from 'os'
import rimraf from 'rimraf'
import linter from '../lib/main'

const goodPath = path.join(__dirname, 'fixtures', 'files', 'good.js')
const badPath = path.join(__dirname, 'fixtures', 'files', 'bad.js')
const emptyPath = path.join(__dirname, 'fixtures', 'files', 'empty.js')
const fixPath = path.join(__dirname, 'fixtures', 'files', 'fix.js')
const configPath = path.join(__dirname, 'fixtures', 'configs', '.eslintrc.yml')
const importingpath = path.join(__dirname, 'fixtures',
const fixturesDir = path.join(__dirname, 'fixtures')

const goodPath = path.join(fixturesDir, 'files', 'good.js')
const badPath = path.join(fixturesDir, 'files', 'bad.js')
const emptyPath = path.join(fixturesDir, 'files', 'empty.js')
const fixPath = path.join(fixturesDir, 'files', 'fix.js')
const configPath = path.join(fixturesDir, 'configs', '.eslintrc.yml')
const importingpath = path.join(fixturesDir,
'import-resolution', 'nested', 'importing.js')
const badImportPath = path.join(__dirname, 'fixtures',
const badImportPath = path.join(fixturesDir,
'import-resolution', 'nested', 'badImport.js')
const ignoredPath = path.join(__dirname, 'fixtures',
'eslintignore', 'ignored.js')
const modifiedIgnorePath = path.join(__dirname, 'fixtures',
const ignoredPath = path.join(fixturesDir, 'eslintignore', 'ignored.js')
const modifiedIgnorePath = path.join(fixturesDir,
'modified-ignore-rule', 'foo.js')
const modifiedIgnoreSpacePath = path.join(__dirname, 'fixtures',
const modifiedIgnoreSpacePath = path.join(fixturesDir,
'modified-ignore-rule', 'foo-space.js')
const endRangePath = path.join(fixturesDir, 'end-range', 'no-unreachable.js')

describe('The eslint provider for Linter', () => {
const { spawnWorker } = require('../lib/helpers')
Expand Down Expand Up @@ -342,4 +344,21 @@ describe('The eslint provider for Linter', () => {
)
})
})

it('handles ranges in messages', () =>
waitsForPromise(() =>
atom.workspace.open(endRangePath).then(editor =>
lint(editor).then((messages) => {
const expected = '<a href=http://eslint.org/docs/rules/no-unreachable ' +
'class="badge badge-flexible eslint">no-unreachable</a> ' +
'Unreachable code.'
expect(messages[0].type).toBe('Error')
expect(messages[0].text).not.toBeDefined()
expect(messages[0].html).toBe(expected)
expect(messages[0].filePath).toBe(endRangePath)
expect(messages[0].range).toEqual([[5, 2], [6, 15]])
})
)
)
)
})
8 changes: 8 additions & 0 deletions src/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,11 @@ export function idsToIgnoredRules(ruleIds = []) {
return ids
}, {})
}

export function validatePoint(textEditor, line, col) {
const buffer = textEditor.getBuffer()
// Clip the given point to a valid one, and check if it equals the original
if (!buffer.clipPosition([line, col]).isEqual([line, col])) {
throw new Error(`${line}:${col} isn't a valid point!`)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced that throwing an error is the right thing to do here. We could be more graceful and just use the clipped position. I suppose the positive here is that we might be able to catch ESLint errors, whereas without throwing an error people will just live with the possibly slightly-inaccurate location marking. Pros and cons either way, but personally I think I lean towards not throwing an error. Curious to hear your thoughts.

Copy link
Member Author

@Arcanemagus Arcanemagus Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This should be validating the point in a similar manner to how rangeFromLineNumber() does. Where this is being called over here is wrapped in the same try/catch that the call to rangFromLineNumber() is, so this throw will be caught and handled in the exact same manner as any other invalid point we run into.

}
}
24 changes: 19 additions & 5 deletions src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import ruleURI from 'eslint-rule-documentation'
// eslint-disable-next-line import/no-extraneous-dependencies, import/extensions
import { CompositeDisposable, Range } from 'atom'

import { spawnWorker, showError, idsToIgnoredRules } from './helpers'
import { spawnWorker, showError, idsToIgnoredRules, validatePoint } from './helpers'

// Configuration
const scopes = []
Expand Down Expand Up @@ -65,6 +65,7 @@ module.exports = {
'linter-eslint:debug': () => {
const textEditor = atom.workspace.getActiveTextEditor()
const filePath = textEditor.getPath()
// eslint-disable-next-line import/no-dynamic-require
const linterEslintMeta = require(Path.join(atom.packages.resolvePackagePath('linter-eslint'), 'package.json'))
const config = atom.config.get('linter-eslint')
const configString = JSON.stringify(config, null, 2)
Expand All @@ -77,6 +78,7 @@ module.exports = {
const detail = [
`atom version: ${atom.getVersion()}`,
`linter-eslint version: ${linterEslintMeta.version}`,
// eslint-disable-next-line import/no-dynamic-require
`eslint version: ${require(Path.join(response.path, 'package.json')).version}`,
`hours since last atom restart: ${Math.round(hoursSinceRestart * 10) / 10}`,
`platform: ${process.platform}`,
Expand Down Expand Up @@ -181,7 +183,9 @@ module.exports = {
*/
return null
}
return response.map(({ message, line, severity, ruleId, column, fix }) => {
return response.map(({
message, line, severity, ruleId, column, fix, endLine, endColumn }
) => {
const textBuffer = textEditor.getBuffer()
let linterFix = null
if (fix) {
Expand All @@ -195,10 +199,20 @@ module.exports = {
}
}
let range
const msgLine = line - 1
try {
range = Helpers.rangeFromLineNumber(
textEditor, line - 1, column ? column - 1 : column
)
if (typeof endColumn !== 'undefined' && typeof endLine !== 'undefined') {
// Here we always want the column to be a number
const msgCol = Math.max(0, column - 1)
validatePoint(textEditor, msgLine, msgCol)
validatePoint(textEditor, endLine - 1, endColumn - 1)
range = [[msgLine, msgCol], [endLine - 1, endColumn - 1]]
Copy link
Member

Choose a reason for hiding this comment

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

Question: why are you subtracting 1 from the endColumn? I understand subtracting for endLine, since it is 1-based, but endColumn is 0-based.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed endColumn is 1-based like all the other points we get from ESLint... did they really make 1/4 of the points 0-based?!?

Copy link
Member

Choose a reason for hiding this comment

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

column numbers are 0-based, line numbers are 1-based.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like rules have mixed 1-based and 0-based coords, but the results are all 1-based.

} else {
// We want msgCol to remain undefined if it was initially so
// `rangeFromLineNumber` will give us a range over the entire line
const msgCol = typeof column !== 'undefined' ? column - 1 : column
range = Helpers.rangeFromLineNumber(textEditor, msgLine, msgCol)
}
} catch (err) {
throw new Error(
`Cannot mark location in editor for (${ruleId}) - (${message})` +
Expand Down