Expand configuration options #115

mdeboer · 2017-09-12T21:37:06Z

As I was getting sick and tired of constantly getting the error message that PHPMD could not be executed in my projects because of a missing phpmd.xml, I decided to make the changes that have already been made to linter-phpcs 😄

Added more configuration options (much like linter-phpcs)
Added .editorconfig
Updated outdated contributing guidelines
Added more tests

Fixes #102.
Fixes #106.

mdeboer · 2017-09-12T21:51:03Z

I suggest this to be released as v1.6.0 as configuration options aren't backwards compatible.

Once this PR is merged I can make another PR for #24 and add the option to configure when to lint,
on change or on-the-fly 😄 Maybe, just maybe do the same for linter-phpcs?

Arcanemagus

Here's what I see just looking over the code.

Arcanemagus · 2017-09-12T22:32:35Z

.editorconfig

+insert_final_newline = true
+indent_style = space
+indent_size = 2
+trim_trailing_whitespace = true


🐛 Apparently you didn't have this file there when editing it, as this is missing the trailing newline 😉.

I did, but as you can see, it doesn't include an entry for a .editorconfig 😉

Arcanemagus · 2017-09-12T22:35:26Z

lib/main.js


    this.subscriptions = new CompositeDisposable();
+
+    // FIXME: Remove after a few versions


🔥 This setting never existed here, as such we don't need to include code removing it.

I took this from linter-phpcs, don't look at me 😉

Arcanemagus · 2017-09-12T22:38:53Z

lib/main.js

+        }
+
+        loadDeps();
+        const fileDir = path.dirname(filePath);


You'll need a check that filePath isn't null, although it should be impossible, sometimes linter will call Providers with TextEditors that don't have this defined (usually on restoring a session on restarting Atom).

Okay, this needs to be fixed in linter-phpcs as well then, as far as I can remember.

Yea, many of the providers still need the guard added to them, for some reason it's rarely an issue for most providers (I think it's a race condition in the TextEditor initializing and maybe they are slow enough it's not an issue? 😕).

Arcanemagus · 2017-09-12T22:40:50Z

package-lock.json

@@ -0,0 +1,1620 @@
+{


🔥 Until Greenkeeper figures out updating of this automatically in their main integration I'm holding off on adding this to more repositories as currently it is a very manual process that gets forgotten. The version of npm in apm doesn't even utilize this anyway.

Ah okay, I'll remove it 👍

Arcanemagus · 2017-09-12T22:42:58Z

lib/main.js

      scope: 'file',
-      lintOnFly: false,
+      lintsOnChange: true,


🔥🐛 This is still a v1 provider, so this needs to stay lintOnFly. Also, as we aren't passing in the file contents this needs to stay false!

Arcanemagus · 2017-09-12T22:45:02Z

lib/main.js

    this.subscriptions.dispose();
  },

  provideLinter() {
    return {
      name: 'PHPMD',
-      grammarScopes: ['text.html.php', 'source.php'],
+      grammarScopes,


🔥 Since there is no need to change the scopes, let's just keep these as statically declared here.

Took this straight from linter-phpcs, I'm not familiar with these kind of packages and the comment made sense.

Arcanemagus · 2017-09-12T22:48:05Z

lib/main.js

          }
        }

+        // Check if a rulesets file exists and handle it
+        const confFileNames = ['phpmd.xml', 'phpmd.xml.dist', 'phpmd.ruleset.xml'];
+        const confFile = await helpers.findAsync(fileDir, confFileNames);


🐎 Put this behind a check on this.autoConfigSearch, there's no point in spending the I/O time searching for these files if we aren't going to use them!

The rulesets declaration down on 114 could probably be moved partially into here as well.

Makes sense 👍

Arcanemagus · 2017-09-12T22:56:13Z

lib/main.js

        };

-        const output = await helpers.exec(executablePath, parameters, options);
+        if (confFile) {


Let's move this up underneath the projectPath logic, and do similar logic there. That way all the cwd modifying logic is in one place. It might make sense to have both set a cwd variable instead.

Sure thing, I'll have a look!

Arcanemagus · 2017-09-12T22:59:07Z

lib/main.js

+  }
+}
+
+// Local variables


🔥 this, see below for reasoning.

Arcanemagus · 2017-09-12T23:06:22Z

.editorconfig

@@ -0,0 +1,7 @@
+root = true


Can you change this to the version used elsewhere? (this).

Sure no problem 👍

Arcanemagus · 2017-09-12T23:07:32Z

Breaking changes in the configuration would need a major release btw.

mdeboer · 2017-09-13T00:08:13Z

I'll get on with the changes @Arcanemagus, thank you for your feedback and I'll update this PR tomorrow 👍

mdeboer · 2017-09-16T15:11:33Z

It took me a little longer, things came up but I think I fixed it all without breaking anything 😉

Arcanemagus

Almost there!

Arcanemagus · 2017-09-16T20:49:54Z

.editorconfig

+insert_final_newline = true
+
+[*.md]
+trim_trailing_whitespace = false


Still missing the ending newline 😛.

Note, you can change the whitespace package's options to handle this for you automatically.

~~Fial.~~ Fail.

Arcanemagus · 2017-09-16T20:51:19Z

lib/main.js

-          const rulesetPath = await helpers.findAsync(filePath, rulesets);
-          if (rulesetPath !== null) {
-            ruleset = rulesetPath;
+        if (fileText === '' || filePath === null) {


It can also be undefined, this is one of the cases where it should just be if (fileText === '' || !filePath).

Arcanemagus · 2017-09-16T20:52:24Z

lib/main.js

      }),
-      atom.config.observe('linter-phpmd.rulesets', (value) => {


Remember how I said to remove that check on the executableTimeout from phpcs? We actually need the same code, but for this setting, sorry!

Hmm, working on multiple projects so I'll have to dig into that to see what you mean 😜

Ah yes, to make it backwards compatbile! 👍

Arcanemagus · 2017-09-16T20:53:03Z

package.json

+      "default": -1,
+      "minimum": -1,
+      "description": "Rule priority threshold; rules with lower priority than this will not be used.",
+      "order": 7


🐛 5 -> 7?

Ah yes, I must have removed an option and not updated the order.

mdeboer · 2017-09-18T20:13:48Z

Okay done! 👍

Arcanemagus · 2017-09-18T23:04:55Z

lib/main.js

+
+    // Backwards compatibility
+    if (atom.config.get('linter-phpmd.rulesets') !== undefined) {
+      this.rulesOrConfigFile = atom.config.get('linter-phpmd.rulesets');


atom.config.set('linter-phpmd.rulesOrConfigFile', atom.config.get('linter-phpmd.rulesets'));

We want to save their old setting, not grab it this one time and then lose it forever 😉.

Doh, you're right 🙄 where's my head at? 😜

mdeboer · 2017-09-18T23:54:00Z

There you go, that should be it. Lesson learnt on linters, thanks! Possibly hate writing them more than using them 😜

Arcanemagus

LGTM, thanks for working through this!

Arcanemagus · 2017-09-19T03:07:45Z

Published as v2.0.0, thanks again!

mdeboer · 2017-09-19T14:01:26Z

Awesome, you're very welcome! Thanks again for your directions 👍 Next on the list is probably being able to lint on-the-fly but we'll see, first have to get back to work 😆

Arcanemagus · 2017-09-19T15:49:28Z

I'm not sure I'm a fan personally of the way to get linters like this to lint on the fly. The only way that has been figured out so far is to create a temporary file from the current contents, and then lint that. This only works on simple linters that have no dependency on the state of the rest of the project, and causes a lot of I/O.

mdeboer added 2 commits September 12, 2017 23:30

Add configuration options much like linter-phpcs

340f098

Add editorconfig and update contributing guidelines

0c6f1bb

mdeboer mentioned this pull request Sep 12, 2017

Only lint when phpmd.xml exists #102

Closed

Fix travis

a5adaaf

mdeboer mentioned this pull request Sep 12, 2017

Project specific configuration #106

Closed

Arcanemagus suggested changes Sep 12, 2017

View reviewed changes

Arcanemagus added bug enhancement labels Sep 12, 2017

Make requested changes

b6e544a

Arcanemagus suggested changes Sep 16, 2017

View reviewed changes

Add requested changes

3402013

Arcanemagus suggested changes Sep 18, 2017

View reviewed changes

Actually save the BC config option, doh!

f1e78af

Arcanemagus approved these changes Sep 19, 2017

View reviewed changes

Arcanemagus changed the title ~~Update configuration options~~ Expand configuration options Sep 19, 2017

Arcanemagus merged commit 7fc108f into AtomLinter:master Sep 19, 2017


		this.subscriptions = new CompositeDisposable();

		// FIXME: Remove after a few versions

		}),
		atom.config.observe('linter-phpmd.rulesets', (value) => {

Expand configuration options #115

Expand configuration options #115

Conversation

mdeboer commented Sep 12, 2017 • edited by Arcanemagus Loading

mdeboer commented Sep 12, 2017 • edited Loading

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

Arcanemagus Sep 12, 2017 • edited Loading

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 Sep 12, 2017

mdeboer commented Sep 13, 2017

mdeboer commented Sep 16, 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

mdeboer commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdeboer commented Sep 18, 2017

Arcanemagus left a comment

Choose a reason for hiding this comment

Arcanemagus commented Sep 19, 2017

mdeboer commented Sep 19, 2017

Arcanemagus commented Sep 19, 2017

mdeboer commented Sep 12, 2017 •

edited by Arcanemagus

Loading

mdeboer commented Sep 12, 2017 •

edited

Loading

Arcanemagus Sep 12, 2017 •

edited

Loading