Use getRules and rule URL metadata where available #1067

Arcanemagus · 2018-01-07T07:54:45Z

Refactor the worker to send the full metadata instead of just the fixable rules, allowing additional functionality to be implemented in the main package code.

Rule gathering from the ESLint instance has been expanded to take advantage of the new CLIEngine#getRules method if it is available.

The rule URL is now pulled directly from the metadata (rule.meta.docs.url) if it is available, falling back to the current method of using eslint-rule-documentation if the rule doesn't specify this for itself.

These new features were added in ESLint v4.15.0.

This also refactors how rules are handled a bit so the following behavior is how it works now:

On each lint in the worker:
- The list of known rules is checked against what ESLint just used to lint
- If it changed, the entire list is sent back to the main thread (later optimization to only send diff? We have had issues with total message size...)
- If it didn't change, only the regular results are sent back
In the main thread (helpers.js):
- When a response from a job contains updated rules, an instance of the new Rules object is updated with the new set of rules
- Whenever a lint job is requested and the user has "Ignore fixable rules" enabled this Rules instance is queried for the current list of fixable rules.
- Whenever messages are being processed the Rules instance is queried for a URL for the rule's documentation.

Arcanemagus · 2018-01-07T08:04:05Z

If you would prefer that the refactor work is split out to a separate PR from the getRules / URL changes just let me know @IanVS.

IanVS · 2018-01-15T04:13:17Z

@Arcanemagus I hate to say it, but yeah if you could split out the refractor work that would be helpful. If you want to keep it in this PR, that's fine, but having a separate commit would be good I think.

IanVS

Thanks for splitting up the work, that was very helpful. I like the PR overall, and the specs are great to have. I do have some ideas of ways that I think we could improve some of this, not all of which was added in this PR but this might be a nice time to clean some of it up.

IanVS · 2018-01-16T02:27:26Z

src/helpers.js

+ * Process the updated rules into the local Map and call further update functions
+ * @param  {Array} updatedRules Array of Arrays of the rule and properties
+ */
+export function updateRules(updatedRules) {


Preference

I'm not sure update is the best name here, because it kind of makes it sound like like the old rules are being kept and just somehow modified. I know this is niggling, but I think I'd prefer a name like replaceRules in this case.

I went with update since as far as anything outside this function is concerned it is an update. The fact that it happens to clear the current contents and then fill in new ones is an implementation detail.

I see your reasoning. Personally, if I call updateThings(newThing), it isn't immediately clear as a consumer of the function whether all the old things will be gone or not. I might think that I'm just adding a new thing to the existing things. I guess all I'm saying is that update is vague, and I find replace to be clearer, but I'm only at a 3 of 10 on the SSOGAF.

At the very least, perhaps the docblock can be clarified that the updatedRules provided will replace the existing rules?

It's certainly not ideal that it clears the old rules in place as a side-effect before updating them. But the general concept of full replacement as an update is totally valid, and the foundation of immutability and great tools like Redux.

I have no issue with doing it that way, just the naming.

Updating in place gets complex since then you have to determine what you need to remove, and what you need to insert. Since there is nothing actually tied to the contents here a simple replacement works just fine.

I'd say I'm at around a 4 or 5 on the SSOGAF 😛.

Haha, you called it a _replace_ment. 😝

IanVS · 2018-01-16T02:29:47Z

src/helpers.js

+/**
+ * Update the list of fixable rules
+ */
+function updateFixableRules() {


What do you think about this function taking fixableRules as an argument, to minimize the side effects and avoid mutating something outside of its own scope? I realize that if you pass it in and modify it, that's still technically a side effect, but it feels slightly cleaner.

IanVS · 2018-01-16T02:32:42Z

src/helpers.js

@@ -8,6 +8,7 @@ import cryptoRandomString from 'crypto-random-string'
 // eslint-disable-next-line import/no-extraneous-dependencies, import/extensions
 import { Range } from 'atom'

+const lintRules = new Map()


It feels to me like if we go this route, lintRules should be a property on a class, and updateFixableRules, updateRules, and getRules should be methods on that class. Normally I'd advocate for a more functional style, but I find maps to be difficult to work with in pure functions.

IanVS · 2018-01-16T02:43:50Z

src/worker-helpers.js

+  const newRuleIds = new Set(newRules.keys())
+
+  // Check for new rules added since the last time we sent currentRules
+  const newRulesIds = new Set(newRules.keys())


Is it intentional to have newRuleIds and newRulesIds? If so, that's pretty confusing, IMHO.

I like the approach to comparing Sets laid out in this SO answer: https://stackoverflow.com/a/44827922/1435658

areSetsEqual = (a, b) => a.size === b.size && [...a].every(value => b.has(value));

It's definitely confusing at best, and that's much cleaner. I'll switch it to something like that 😉.

IanVS · 2018-01-16T02:45:22Z

src/worker.js

-  // to check the loaded rules (including plugin rules) and update our list of fixable rules.
-  updateFixableRules(cliEngine.linter)
+  const rules = Helpers.getRules(cliEngine)
+  sendRules = Helpers.didRulesChange(rulesMetadata, rules)


Preference

I know this wasn't created in this PR, but as a boolean constant, let's name this something like shouldSendRules.

IanVS · 2018-01-16T02:48:01Z

src/worker.js

+        // You can't emit Maps, convert to Array of Arrays to send back.
+        response.updatedRules = []
+        rulesMetadata.forEach((props, rule) =>
+          response.updatedRules.push([rule, props]))


Doesn't Array.from(rulesMetadata) do what you want here? And if not, can we use rulesMetadata.entries()?

Honestly I didn't even think to try it. Array.from(<some Map>) does exactly this. 👍

IanVS · 2018-01-16T05:44:56Z

src/rules.js

+  constructor(newRules) {
+    this.rules = new Map()
+    if (newRules) {
+      this.updateRules(newRules)


I'm no Map expert, but couldn't this be done in one step? Something like:

this.rules = new Map(newRules)

Since the constructor will never be called with a parameter except in specs, sure. If this was a "real" class though we would want validation in there that whatever newRules is is something that actually makes sense to instantiate with.

Refactor the worker to send the full metadata instead of just the fixable rules, allowing additional functionality to be implemented in the main package code.

The rule URL is now pulled directly from the metadata if it is available, falling back to the current method of using `eslint-rule-documentation` if the rule doesn't specify this for itself.

Rule gathering from the ESLint instance has been expanded to take advantage of the new `CLIEngine#getRules` method if it is available.

Rename `sendRules` to `shouldSendRules` to make the purpose of the flag more clear.

Use a much simpler logic to determine whether the rule `Map`s are equal or not. Also fixes up the docblock a bit.

Apparently `Array.from` is quite happy taking a `Map` object directly.

Pull all the logic related to storing the list of known rules and accessing it out into a class of its own.

IanVS · 2018-01-18T03:44:07Z

I'd still prefer replaceRules to updateRules, but I won't block a merge over it. :)

Check that the given Array contains Arrays that "look" like the exected format: [String, Object]. Also renames `updatedRules` to `replaceRules` since it is now taking advantage of the Map constructor to parse through the Array.

Arcanemagus added the enhancement label Jan 7, 2018

Arcanemagus self-assigned this Jan 7, 2018

Arcanemagus requested a review from IanVS January 7, 2018 07:54

Arcanemagus force-pushed the arcanemagus/rule-url branch from 363b9d7 to cb283ba Compare January 7, 2018 08:02

Arcanemagus force-pushed the arcanemagus/rule-url branch 2 times, most recently from 757e82f to e3af87f Compare January 7, 2018 08:32

This was referenced Jan 8, 2018

Add URL to rule documentation to the metadata eslint-community/eslint-plugin-eslint-plugin#54

Closed

Breaking: Make require('eslint').linter non-enumerable (fixes #9270) eslint/eslint#9692

Merged

Arcanemagus force-pushed the arcanemagus/rule-url branch 2 times, most recently from 69661f7 to c8cf6a5 Compare January 15, 2018 23:32

IanVS suggested changes Jan 16, 2018

View reviewed changes

Arcanemagus force-pushed the arcanemagus/rule-url branch from efa3a44 to afc5af5 Compare January 16, 2018 05:43

IanVS reviewed Jan 16, 2018

View reviewed changes

Arcanemagus force-pushed the arcanemagus/rule-url branch from afc5af5 to 53f1494 Compare January 17, 2018 17:47

Arcanemagus added 7 commits January 17, 2018 18:20

Send full rule metadata back from the worker

1c2d394

Refactor the worker to send the full metadata instead of just the fixable rules, allowing additional functionality to be implemented in the main package code.

Utilize URL from rule metadata

6037f5e

The rule URL is now pulled directly from the metadata if it is available, falling back to the current method of using `eslint-rule-documentation` if the rule doesn't specify this for itself.

Use CLIEngine#getRules when available

57542e6

Rule gathering from the ESLint instance has been expanded to take advantage of the new `CLIEngine#getRules` method if it is available.

sendRules -> shouldSendRules

caf7d12

Rename `sendRules` to `shouldSendRules` to make the purpose of the flag more clear.

Simplify the logic behind didRulesChange

ec95f43

Use a much simpler logic to determine whether the rule `Map`s are equal or not. Also fixes up the docblock a bit.

Use Array.from on the rule Map

6096bd4

Apparently `Array.from` is quite happy taking a `Map` object directly.

Move rule related logic to a class

03af716

Pull all the logic related to storing the list of known rules and accessing it out into a class of its own.

Arcanemagus force-pushed the arcanemagus/rule-url branch from 53f1494 to 03af716 Compare January 18, 2018 02:20

IanVS approved these changes Jan 18, 2018

View reviewed changes

Validate rules before replacing the current set

deb5c31

Check that the given Array contains Arrays that "look" like the exected format: [String, Object]. Also renames `updatedRules` to `replaceRules` since it is now taking advantage of the Map constructor to parse through the Array.

Arcanemagus merged commit d2129ff into master Jan 18, 2018

Arcanemagus deleted the arcanemagus/rule-url branch January 18, 2018 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getRules and rule URL metadata where available #1067

Use getRules and rule URL metadata where available #1067

Arcanemagus commented Jan 7, 2018 •

edited

Loading

Arcanemagus commented Jan 7, 2018

IanVS commented Jan 15, 2018

IanVS left a comment

IanVS Jan 16, 2018

Arcanemagus Jan 16, 2018

IanVS Jan 16, 2018 •

edited

Loading

skylize Jan 16, 2018

IanVS Jan 16, 2018

Arcanemagus Jan 16, 2018

IanVS Jan 16, 2018

IanVS Jan 16, 2018

IanVS Jan 16, 2018

IanVS Jan 16, 2018

Arcanemagus Jan 16, 2018

IanVS Jan 16, 2018

Arcanemagus Jan 16, 2018

IanVS Jan 16, 2018

Arcanemagus Jan 16, 2018

IanVS Jan 16, 2018

Arcanemagus Jan 16, 2018

IanVS commented Jan 18, 2018

Use getRules and rule URL metadata where available #1067

Use getRules and rule URL metadata where available #1067

Conversation

Arcanemagus commented Jan 7, 2018 • edited Loading

Arcanemagus commented Jan 7, 2018

IanVS commented Jan 15, 2018

IanVS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS Jan 16, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS commented Jan 18, 2018

Arcanemagus commented Jan 7, 2018 •

edited

Loading

IanVS Jan 16, 2018 •

edited

Loading