ES2017 Rewrite #113

Arcanemagus · 2016-10-03T20:23:28Z

Major rewrite in ES2015, including:

Clicking a rule name will now open the documentation
Disable on the fly linting!
- The file copying method used before wasn't complete enough to cover all edge cases and was just causing problems.
Fix a race condition where file contents are changed after the lint is triggered
Move configuration to configSchema in package.json

The new regex was validated here: https://regex101.com/r/EVwEiU/1

Fixes #30.
Fixes #16.
Closes #13.
Fixes #12.

Major rewrite in ES2015, including: * Clicking a rule name will now open the documentation * Disable on the fly linting! * The file copying method used before wasn't complete enough to cover all edge cases and was just causing problems. * Fix a race condition where file contents are changed after the lint is triggered * Move configuration to `configSchema` in `package.json`

Arcanemagus · 2016-10-03T20:31:23Z

@jschroeder9000 If you're still active here I'd love to have this looked over and your thoughts on the changes it is making.

Arcanemagus · 2016-10-03T20:42:37Z

spec/fixtures/.haml-lint.yml

+    ignored_cops:
+      - Style/FinalNewline
+      - Style/TrailingBlankLines
+      - Style/EndOfLine


For some reason this rule was triggering on all of the files for me, causing issues for the "valid" one. Unfortunately even though the other two are defined in the defaults and shouldn't be necessary here, it doesn't work without them.

Arcanemagus · 2016-10-03T20:43:04Z

package.json

  "license": "MIT",
  "engines": {
-    "atom": ">=1.0.0 <2.0.0"
+    "atom": ">=1.4.0 <2.0.0"


Atom v1.4.0 was the first version to support the configSchema.

steelbrain · 2016-10-03T21:08:56Z

lib/main.js

+        // Add the file to be linted
+        parameters.push(filePath);
+
+        return helpers.exec(executablePath, parameters, options).then((output) => {


Can you make it an async function and await it instead? It'll save you from deeper nesting for so many lines

steelbrain · 2016-10-03T21:09:15Z

lib/main.js

+                'to update the results.',
+              dismissable: true,
+            };
+            atom.notifications.addWarning(warnMessage, warnOptions);


This should be a console warning because this is too loud (as discussed privately)

steelbrain · 2016-10-03T21:10:51Z

lib/main.js

+          }
+
+          const messages = [];
+          let match = regex.exec(output);


Code is duplicated here and at at line 94. You could make it like while ((match = regex.exec(output)) !== null )

That means you are assigning in the conditional of the while, which is against the code style guide.

I personally don't think it's wrong. Besides the code style guide was not given to us by God so we are free to object and change it to something that helps us keep the codebase clean and unduplicated

Well, I personally agree with the code style guide so I'm keeping it the way it is 😛.

steelbrain · 2016-10-03T21:11:07Z

package.json

+    "atom": ">=1.4.0 <2.0.0"
+  },
+  "configSchema": {
+    "hamlLintExecutablePath": {


Make it just executablePath

Since this will be a major release, that sounds like a good idea.

Rename the configuration option to match all the other linters.

As the Atom notification might be too intrusive for a non-critical issue, move this to the console.

Technically this makes it ES2017, but Atom supports this via Babel.

Conflicts: README.md

Arcanemagus · 2016-10-04T17:51:44Z

Giving this another day before merging.

jschroeder9000 · 2016-10-05T01:52:11Z

Thanks for your work on this, @Arcanemagus. I think this is a great start, though I find the decision to remove support for on the fly linting very disappointing.

I see that the linter is no longer using or even looking for a rubocop config at all. That will severely cripple the usefulness of the linter by always linting with rubocop defaults instead of the what the user/project desires.

Haml-lint has added support for setting an environment variable to specify the path to the rubocop config file (HAML_LINT_RUBOCOP_CONF) and I believe that will solve our problems. I think the right thing to do is to search for .rubocop.yml the same way the search for .haml-lint.yml is done and add the appropriate environment variable to the options passed to exec if it is found. No file copying is necessary and I think this should also correctly solve #30.

With that implementation in place and since no file copying is being performed, I think on the fly linting should remain in place.

jschroeder9000 · 2016-10-05T02:22:07Z

Open mouth, insert foot...

Haml-lint will find the .rubocop.yml and use it by itself since the source file is no longer being copied to a temporary location, so ignore my comments about needing to find it and pass an environment variable. I guess the only reason we would need to pass an environment variable would be if we wanted to add an option for a global .rubocop.yml file like we do with .haml-lint.yml. In fact, I don't think we need to be searching for a .haml-lint.yml file anymore either since files aren't being moved; haml-lint will find it by itself and use it. We would only need to pass it in if the global config file is set.

Since there is no longer any file copying and that was the reasoning for disabling on the fly linting, I guess I'm confused as to why it is still being disabled. I think the whole thing with hackily copying config files to a temp directory was a just a relic left over from the original linter plugin always copying files to a temp directory.

Arcanemagus · 2016-10-05T03:04:42Z

The searching for the file is left in to support the "global config file" option, otherwise we don't know if we need to specify our special one from the settings or not.

Since there is no longer any file copying and that was the reasoning for disabling on the fly linting, I guess I'm confused as to why it is still being disabled.

I assumed from the fact that it was being copied in the first place that haml-lint doesn't support getting the contents from stdin, if we can send the current editor contents to it via stdin then of course we can just enable on the fly linting. I guess I never verified that it didn't 😛.

jschroeder9000 · 2016-10-05T04:43:02Z

Ah, I had forgotten the issue with STDIN. It's all coming back to me now. Haml-lint does not support linting from STDIN and I agree simplifying things and disabling on the fly linting is the way to go.

I think this looks good and linking to the documentation is definitely a cool addition.

👍 from me.

Arcanemagus · 2016-10-05T16:24:11Z

Published as v2.0.0! 🎉

Arcanemagus added bug enhancement labels Oct 3, 2016

This was referenced Oct 3, 2016

Rubocop project config is not using by linter-haml #16

Closed

Use atom-linter helpers for dealing with temp files #13

Closed

Arcanemagus commented Oct 3, 2016

View reviewed changes

steelbrain suggested changes Oct 3, 2016

View reviewed changes

Arcanemagus added 4 commits October 3, 2016 14:18

hamlLintExecutablePath -> executablePath

265fe69

Rename the configuration option to match all the other linters.

Change warning to a console message

28c120b

As the Atom notification might be too intrusive for a non-critical issue, move this to the console.

Utilize async/await

a3cf0b4

Technically this makes it ES2017, but Atom supports this via Babel.

Merge branch 'master'

70d7bff

Conflicts: README.md

Arcanemagus changed the title ~~ES2015 Rewrite~~ ES2017 Rewrite Oct 4, 2016

Arcanemagus merged commit 69e45e8 into master Oct 5, 2016

Arcanemagus deleted the es2015-rewrite branch October 5, 2016 16:11

tjvc mentioned this pull request Oct 23, 2017

Linting temporary file breaks haml-lint RuboCop integration dense-analysis/ale#1035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES2017 Rewrite #113

ES2017 Rewrite #113

Arcanemagus commented Oct 3, 2016 •

edited

Loading

Arcanemagus commented Oct 3, 2016

Arcanemagus Oct 3, 2016

Arcanemagus Oct 3, 2016

steelbrain Oct 3, 2016

steelbrain Oct 3, 2016

steelbrain Oct 3, 2016

Arcanemagus Oct 3, 2016

steelbrain Oct 3, 2016

Arcanemagus Oct 3, 2016

steelbrain Oct 3, 2016

Arcanemagus Oct 3, 2016

Arcanemagus commented Oct 4, 2016

jschroeder9000 commented Oct 5, 2016

jschroeder9000 commented Oct 5, 2016

Arcanemagus commented Oct 5, 2016

jschroeder9000 commented Oct 5, 2016

Arcanemagus commented Oct 5, 2016

ES2017 Rewrite #113

ES2017 Rewrite #113

Conversation

Arcanemagus commented Oct 3, 2016 • edited Loading

Arcanemagus commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus commented Oct 4, 2016

jschroeder9000 commented Oct 5, 2016

jschroeder9000 commented Oct 5, 2016

Arcanemagus commented Oct 5, 2016

jschroeder9000 commented Oct 5, 2016

Arcanemagus commented Oct 5, 2016

Arcanemagus commented Oct 3, 2016 •

edited

Loading