Adding command to fix TS files + other changes (see desc.) #149

Xapphire13 · 2017-03-29T19:27:31Z

Adding command to fix TS files.
Added option to skip .d.ts files.
Adding linter name so that it appears in the history log.

…ing linter name so that it appears in the history log.

Arcanemagus

Here's a first pass review. I'm still not entirely sure about adding classes to the editors, but I can't see any harm to it and it does let the command only show up for files where it would make sense, which is a nice touch.

Arcanemagus · 2017-03-29T22:37:37Z

lib/main.js

      atom.config.observe('linter-tslint.useLocalTslint', (use) => {
        tslintCache.clear();
        this.useLocalTslint = use;
      }),
    );
+
+    atom.workspace.observeActivePaneItem((activeItem) => {


Why observeActivePaneItem instead of observeTextEditors?

Lack of API knowledge =], will update, thanks

Arcanemagus · 2017-03-29T22:41:47Z

lib/main.js

+ * @return {boolean} Whether or not the grammar is for TypeScript
+ */
+function isTypescriptGrammar(grammar) {
+  return grammar.fileTypes.find(fileType => /.*ts/i.test(fileType));


Please put a note that this is not part of the public API if we can't find a way around using this.

Could you elaborate what you mean by non-public API here? This is my first modification of an Atom package and am unaware of how else to do helper methods.

Anything that isn't listed in the Atom Docs isn't part of the public API, and is subject to change at any time 😉.

In this case the Grammar object you are accessing is very bare bones, and doesn't expose a way to get to the fileTypes property you are utilizing here. This comment shows a way around all this 😉.

Got it! Thanks for the detailed explanation

Arcanemagus · 2017-03-29T22:47:35Z

lib/main.js

+      atom.commands.add(`atom-text-editor.${editorClass}`, {
+        'linter-tslint:fix-file': () => {
+          const textEditor = atom.workspace.getActiveTextEditor();
+          // const filePath = textEditor.getPath();


No reason to bring in commented out code.

Oops, I missed that thanks!

Arcanemagus · 2017-03-29T22:50:38Z

lib/main.js

+
+    atom.workspace.observeActivePaneItem((activeItem) => {
+      if (activeItem && activeItem.getGrammar) {
+        if (isTypescriptGrammar(activeItem.getGrammar())) {


Instead of using a non-public API can we validate it based on scopes like everything else with linter providers is done?

You can find an example of how to use the cursors to get the scopes and check it using the public API from linter-eslint here.

Went with a solution i found in the Atom api that seemed simpler.

Arcanemagus · 2017-03-29T22:52:31Z

lib/main.js

+          if (!textEditor || textEditor.isModified()) {
+            // Abort for invalid or unsaved text editors
+            atom.notifications.addError('Linter-TSLint: Please save before fixing');
+            return Promise.resolve();


Just mark the function as async here and all return values will be Promises. You can also use await on the lint() call, as well as getting rid of the chained then() calls.

I though async/await didn't make the ES6 spec and wasn't going to have wide support until ECMA 2017. I will change and test it out though

Atom has built in transpilation of all package files that start with 'use babel';, as well as having a relatively new version of Node.js, so new features like that are usable in Atom.

Arcanemagus · 2017-03-29T22:54:20Z

lib/main.js

-              rulesDirectory.push(this.rulesDirectory);
-            }
-          }
+        if (atom.config.get('linter-tslint.ignoreTypings') && /.*\.d\.ts$/i.test(textEditor.getPath())) {


This setting should be observe()'d like the others.

Also, how about this for the second part? If the case-insensitivity is important add a .toLowerCase() in there.

textEditor.getPath().endsWith('.d.ts')

Arcanemagus · 2017-03-29T23:39:30Z

Thanks for taking this on btw!

I don't use TypeScript myself, and it seems the people that were maintaining this have abandoned it unfortunately so it's needed some attention for a while.

Xapphire13 · 2017-04-03T20:18:22Z

I have updated with the changes requested. Is anything else needed from me now? (Still new to GitHub PRs)

Arcanemagus

More thorough review now that I'm sure the command thing is something that should happen 😉.

Some of this might just be inherited issues exposed due to moving the lint() code out into it's own function, but we might as well fix it now.

Arcanemagus · 2017-04-04T22:45:34Z

lib/main.js

+    // we can enable commands only for TypeScript editors.
+    this.subscriptions.add(
+      atom.workspace.observeTextEditors((textEditor) => {
+        if (textEditor.getRootScopeDescriptor().scopes.some(scope => scope === typescriptScope)) {


Instead of accessing .scopes directly, use getScopesArray().

Also, the scopes allowed by the provider here should be used, so typescriptScope should rather be typeScriptScopes and be an array equal to those scopes (the definiton down there should just be a reference to the same array), then the check function should be:

textEditor.getRootScopeDescriptor().getScopesArray().some(scope => typeScriptScopes.includes(scope))

(This is pushing this line rather long, might want to split it up)

Arcanemagus · 2017-04-04T22:50:03Z

lib/main.js

@@ -8,6 +8,8 @@ import fs from 'fs';
 import requireResolve from 'resolve';

 const TSLINT_MODULE_NAME = 'tslint';
+const typescriptScope = 'source.ts';
+const editorClass = 'typescript-editor';


Let's make this very specific, maybe something like linter-tslint-compatible-editor?

Arcanemagus · 2017-04-04T22:56:35Z

lib/main.js

+    this.subscriptions.add(
+      atom.workspace.observeTextEditors((textEditor) => {
+        if (textEditor.getRootScopeDescriptor().scopes.some(scope => scope === typescriptScope)) {
+          textEditor.element.classList.add(editorClass);


This definitely isn't compatible with the public API, does this get you what you need?

atom.views.getView(textEditor);

That should at least get you the proper DOM element through the API.

Arcanemagus · 2017-04-04T23:00:58Z

lib/main.js

+
+          // The fix replaces the file content and the cursor jumps automatically
+          // to the beginning of the file, so save current cursor position
+          const cursorPosition = textEditor.getCursorBufferPosition();


Just a note: This only happens sometimes, and when it does happen, restoring the cursor like this for some reason doesn't seem to work from the limited testing from AtomLinter/linter-eslint#853 (it hasn't been pushed out to the public yet).

Arcanemagus · 2017-04-04T23:02:00Z

lib/main.js

+          } catch (err) {
+            atom.notifications.addWarning(err.message);
+          } finally {
+            // set cursor to the position before fix job


"set" -> "Set" (or "Restore"?)

Arcanemagus · 2017-04-04T23:06:52Z

lib/main.js

+      const linter = new Linter(Object.assign({
+        formatter: 'json',
+        rulesDirectory,
+      }, options));


Does it handle undefined being passed in here? If not, you need a default specified up on L179.

undefined is handled yes

Arcanemagus · 2017-04-04T23:17:55Z

lib/main.js


-          linter.lint(filePath, text, configuration);
-          const lintResult = linter.getResult();
+  lint(textEditor, options) {


Declare this as async lint(textEditor, options) {...

Arcanemagus · 2017-04-04T23:18:03Z

lib/main.js

-            // Text has been modified since the lint was triggered, tell linter not to update
-            return null;
-          }
+    return this.getLinter(filePath).then((Linter) => {


And then await on this.

(Note that getLocalLinter and getLinter could be re-written in this manner as well if you wanted.)

Arcanemagus · 2017-04-04T23:20:42Z

lib/main.js

-            return [];
+      let { rulesDirectory } = configuration;
+      if (rulesDirectory) {
+        const configurationDir = path.dirname(configurationPath);


dirname will throw a deprecation if configurationPath is null or undefined, is this a possibility? You probably need a check in here to handle that, as well as the usages further down.

This code was already here so I'm not an expert in it's behavior.

Arcanemagus · 2017-04-04T23:21:37Z

lib/main.js

+      let { rulesDirectory } = configuration;
+      if (rulesDirectory) {
+        const configurationDir = path.dirname(configurationPath);
+        if (!Array.isArray(rulesDirectory)) {


Is this for backwards compatibility with older tslint versions? Maybe put a comment in here for the reasoning behind this.

I have no idea, this code was here before my change and is just moving with my refactoring of the method

Arcanemagus

Sorry I missed this till just now!

Arcanemagus · 2017-04-09T20:52:43Z

@Xapphire13 Would you be interested in helping to maintain this package?

Xapphire13 · 2017-04-10T04:14:56Z

@Arcanemagus Yes, I'd love to help maintain this package =]

Arcanemagus · 2017-04-10T16:03:43Z

@Xapphire13 Invite sent, if you have any questions feel free to bug me on Atom's Slack, there is also a #linter and #linter-plugin-dev channel there if I'm not around.

Thanks for taking this up!

Adding command to fix TS files. Added option to skip .d.ts files. Add…

6388edb

…ing linter name so that it appears in the history log.

Arcanemagus suggested changes Mar 29, 2017

View reviewed changes

Xapphire13 added 2 commits March 29, 2017 18:38

Updating changes with PR feedback

6a276ef

Adding TextEditor subscription to the list of subscriptions for cleanup

2d6ef85

Xapphire13 mentioned this pull request Apr 4, 2017

Hiding 'ESLint: Fix file' in non-supported files AtomLinter/linter-eslint#868

Closed

Arcanemagus suggested changes Apr 4, 2017

View reviewed changes

Addressing PR feedback

75af798

Arcanemagus approved these changes Apr 9, 2017

View reviewed changes

Arcanemagus merged commit daf6a23 into AtomLinter:master Apr 9, 2017

Xapphire13 deleted the fixts branch April 10, 2017 17:09

Xapphire13 added the enhancement label Apr 13, 2017

Adding command to fix TS files + other changes (see desc.) #149

Adding command to fix TS files + other changes (see desc.) #149

Conversation

Xapphire13 commented Mar 29, 2017

Arcanemagus left a 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

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 Mar 29, 2017

Xapphire13 commented Apr 3, 2017

Arcanemagus left a 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

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 left a comment

Choose a reason for hiding this comment

Arcanemagus commented Apr 9, 2017

Xapphire13 commented Apr 10, 2017

Arcanemagus commented Apr 10, 2017