Major cleanup #38

lucasdf · 2017-08-26T15:27:08Z

Converted from CoffeeScript to JavaScript
Configured linter and fixed lint issues
Upgraded to API v2
Added specs
Fixed some messages not being displayed
Fixed lintsOnChange was set to true although the package was not checking the content on change

Add specs for the linter. Some tests are failling since they test the issues explained in AtomLinter#37

Set the project to private in NPM. Reorder the properties.

- Modify the regex to detect all types of messages that may be returned by the linter. This fixes AtomLinter#37 - Made some improvements like using `generateRange` for the line range.

johnwebbcole

@lucasdf Looks good, would you mind updating the readme with instructions on running the tests?

Arcanemagus

A few minor little changes. There are several more things that could be done to later improve how this package works, but they aren't necessary and can be saved for later 😉.

Arcanemagus · 2017-08-26T20:35:02Z

lib/init.js

+            const patterns = [
+              {
+                // regex for "ERROR: ENV invalid format ENV_VARIABLE on line 2"
+                regex: /(WARN|ERROR):(.*) on line (\d+)/,


Shouldn't this be /(WARN|ERROR): (.*) on line (\d+)/ to remove the initial space from the message?

Arcanemagus · 2017-08-26T20:35:26Z

lib/init.js

+              },
+              {
+                // regex for "ERROR: Multiple CMD instructions found, only line 3 will take effect"
+                regex: /(WARN|ERROR):(.*), only line (\d+)/,


Same here, remove the initial space from the captured message.

Arcanemagus · 2017-08-26T20:43:53Z

lib/init.js

+
+              messages.push({
+                severity: match.severity === 'WARN' ? 'warning' : 'error',
+                excerpt: match.excerpt.trim(),


Fixing the regex above should remove the need for .trim() here.

Arcanemagus · 2017-08-26T20:45:11Z

lib/init.js

+              {
+                // regex for "ERROR: ENV invalid format ENV_VARIABLE on line 2"
+                regex: /(WARN|ERROR):(.*) on line (\d+)/,
+                cb: m => ({ lineNo: m[3], excerpt: m[2], severity: m[1] }),


Since we're doing arithmetic on it later, do a Number.parseInt(m[3], 10) on the lineNo.

Arcanemagus · 2017-08-26T20:45:24Z

lib/init.js

+              {
+                // regex for "ERROR: Multiple CMD instructions found, only line 3 will take effect"
+                regex: /(WARN|ERROR):(.*), only line (\d+)/,
+                cb: m => ({ lineNo: m[3], excerpt: m[2], severity: m[1] }),


Same here, do a Number.parseInt(m[3], 10) on the lineNo.

Arcanemagus · 2017-08-26T20:49:28Z

spec/linter-docker-spec.js

+    // https://github.com/AtomLinter/Meta/issues/15
+    const activationPromise = atom.packages.activatePackage('linter-docker');
+
+    // await atom.packages.activatePackage('language-python');


Since this package isn't using activationHooks, this whole block could be replaced with just this:

await atom.packages.activatePackage('linter-docker');

If you do want to use the activationHook compatible method just in case it gets implemented later then the right way to do it would be to activate the associated language and open a file of that language. Since opening a file adds ~1 second to each specs time, it's probably best to just go with the above version for now.

- Upgrade linter to v2. - Use `atom-package-deps` to check and install dependencies. - Modify tests to check for v2 messages

The package does not check the content on change, so this must be set to False. dockerlint does not provide a way to pass `stdin`, so in order to support on change lint then temporary files would need to be used.

lucasdf · 2017-08-27T00:11:36Z

@johnwebbcole thanks for reviewing this! I added instructions in the README for running the tests and linting the project.

@Arcanemagus I fixed those issues!

Arcanemagus

LGTM, thanks!

Arcanemagus · 2017-08-27T00:16:48Z

Looks like this should be a patch release since it was never actually linting changes in the first place 😛.

lucasdf · 2017-08-27T00:22:43Z

This should close both #10 and #5

Arcanemagus · 2017-08-27T21:02:14Z

Published in v0.2.1. 🎉

lucasdf · 2017-08-27T21:22:58Z

Great!

lucasdf · 2017-08-27T21:23:39Z

Now we may setup CircleCI!

lucasdf added 5 commits August 26, 2017 13:23

Add specs

3cf5dfe

Add specs for the linter. Some tests are failling since they test the issues explained in AtomLinter#37

Convert to Javascript

40d8970

Configure ESLint and fix lint issues

fc36d57

Set project to private and organize package.json

1b01797

Set the project to private in NPM. Reorder the properties.

Display all types of messages

1294a66

- Modify the regex to detect all types of messages that may be returned by the linter. This fixes AtomLinter#37 - Made some improvements like using `generateRange` for the line range.

lucasdf force-pushed the add_specs branch from bd28474 to 4a0fb1d Compare August 26, 2017 16:26

johnwebbcole approved these changes Aug 26, 2017

View reviewed changes

Arcanemagus added bug enhancement labels Aug 26, 2017

Arcanemagus suggested changes Aug 26, 2017

View reviewed changes

lucasdf added 3 commits August 26, 2017 21:05

Upgrade to v2

f68a59c

- Upgrade linter to v2. - Use `atom-package-deps` to check and install dependencies. - Modify tests to check for v2 messages

Set lintsOnChange to false

fab2101

The package does not check the content on change, so this must be set to False. dockerlint does not provide a way to pass `stdin`, so in order to support on change lint then temporary files would need to be used.

Add instructions for running tests

e1c0ec6

lucasdf force-pushed the add_specs branch from 4a0fb1d to e1c0ec6 Compare August 27, 2017 00:09

Arcanemagus approved these changes Aug 27, 2017

View reviewed changes

Arcanemagus merged commit cbc2ad7 into AtomLinter:master Aug 27, 2017

lucasdf deleted the add_specs branch August 27, 2017 00:25

This was referenced Aug 27, 2017

Implement specs #10

Closed

Can't get it to work #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major cleanup #38

Major cleanup #38

lucasdf commented Aug 26, 2017

johnwebbcole left a comment

Arcanemagus left a comment

Arcanemagus Aug 26, 2017

Arcanemagus Aug 26, 2017

Arcanemagus Aug 26, 2017

Arcanemagus Aug 26, 2017

Arcanemagus Aug 26, 2017

Arcanemagus Aug 26, 2017

lucasdf commented Aug 27, 2017

Arcanemagus left a comment

Arcanemagus commented Aug 27, 2017

lucasdf commented Aug 27, 2017

Arcanemagus commented Aug 27, 2017

lucasdf commented Aug 27, 2017

lucasdf commented Aug 27, 2017

Major cleanup #38

Major cleanup #38

Conversation

lucasdf commented Aug 26, 2017

johnwebbcole left a comment

Choose a reason for hiding this comment

Arcanemagus left a comment

Choose a reason for hiding this comment

Arcanemagus Aug 26, 2017

Choose a reason for hiding this comment

Arcanemagus Aug 26, 2017

Choose a reason for hiding this comment

Arcanemagus Aug 26, 2017

Choose a reason for hiding this comment

Arcanemagus Aug 26, 2017

Choose a reason for hiding this comment

Arcanemagus Aug 26, 2017

Choose a reason for hiding this comment

Arcanemagus Aug 26, 2017

Choose a reason for hiding this comment

lucasdf commented Aug 27, 2017

Arcanemagus left a comment

Choose a reason for hiding this comment

Arcanemagus commented Aug 27, 2017

lucasdf commented Aug 27, 2017

Arcanemagus commented Aug 27, 2017

lucasdf commented Aug 27, 2017

lucasdf commented Aug 27, 2017