New: Allow passing a function as `fix` option (fixes #8039) #8730

IanVS · 2017-06-14T03:26:34Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

Closes #8039

What changes did you make? (Give an overview)
The main change here is to allow the fix option of the CLIEngine to be either a boolean or now also a function which is given each linting message and is expected to return a boolean. This will give users greater control over what kind of fixes are applied by ESLint.

Is there anything you'd like reviewers to focus on?
This may be somewhat incomplete. I worked on it a few months ago, and then put it aside. I've rebased on latest master and believe the happy path works correctly, but there may need to be more safety added around executing the function that is provided as the fix option, as right now all it does is check for truthiness of the return value (which is maybe fine, but is worth some consideration).

I'm happy to continue working on this to get it across the finish line, based on feedback from reviewers. Or alternatively if anyone wants to jump in and take it over, I'm happy to turn it over as well.

eslintbot · 2017-06-14T03:26:35Z

Thanks for the pull request, @IanVS! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

mention-bot · 2017-06-14T03:26:37Z

@IanVS, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @gyandeeps and @roadhump to be potential reviewers.

eslintbot · 2017-06-15T04:02:05Z

LGTM

IanVS · 2017-06-15T04:03:18Z

I've rebased this to account for the recent addition of linter.verifyAndFix().

not-an-aardvark

Thanks! This looks good, I just have a few minor suggestions.

not-an-aardvark · 2017-06-19T02:34:55Z

lib/linter.js

     * @returns {Object} The result of the fix operation as returned from the
     *      SourceCodeFixer.
     */
-    verifyAndFix(text, config, options) {
+    verifyAndFix(text, config, options, shouldFix) {


Nitpick: Could fix be one of the options (third parameter) rather than adding another parameter to this method (e.g. as a fixFilter property)? It would be nice to avoid the boolean trap.

not-an-aardvark · 2017-06-19T02:40:14Z

tests/lib/util/source-code-fixer.js

+                const result = SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_START], shouldFixSpy);
+
+                assert.equal(result.output, TEST_CODE);
+            });


Can you add a test to assert that the fix filter is called as a pure function (without a this value)? I don't want to accidentally expose internal objects because that could make it difficult to change the implementation later if people start to rely on the this value.

not-an-aardvark · 2017-06-19T02:43:52Z

tests/lib/util/source-code-fixer.js

+                const result = SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_START], shouldFixSpy);
+
+                assert.equal(result.output, TEST_CODE);
+            });


Can you add a test that asserts that only the fixes for which the fix filter returns true are applied? All of the filters in the existing tests appear to either always return true or always return false, but as a sanity check it would be useful to make sure the function works when it only sometimes returns true.

not-an-aardvark · 2017-06-19T02:51:40Z

lib/util/source-code-fixer.js

-                output = "";
+            if (typeof shouldFix === "function") {
+                if (shouldFix(problem)) {
+                    fixesWereApplied = attemptFix(problem) || fixesWereApplied;


If I'm understanding correctly, attemptFix will only return false if the current fix conflicts with another fix that has already been applied. In that case, will fixesWereApplied ever be false after this line?

Yep, that's a great point. I can't remember my thinking here, but I think I was trying to be cautious and not build in assumptions about the conditions under which attemptFix would return true or false. But, the diff shows that currently we always return true from this block as it is. I'll be happy to simplify this down, and add a comment about why it is always true.

not-an-aardvark · 2017-06-19T02:52:40Z

lib/util/source-code-fixer.js

-            if ((start < 0 && end >= 0) || (start === 0 && fix.text.startsWith(BOM))) {
-                output = "";
+            if (typeof shouldFix === "function") {
+                if (shouldFix(problem)) {


Nitpick: Maybe this could be simplified to

if (typeof shouldFix !== "function" || shouldFix(problem)) { // ... } else { // ... }

to avoid nesting/duplication.

not-an-aardvark · 2017-06-19T02:53:05Z

lib/util/source-code-fixer.js

+        return {
+            fixed: false,
+            messages,
+            output: ""


Should the output here be sourceCode.text rather than the empty string, since the resulting text is the same as the original text? (I'm not sure whether the other parts of the code check the fixed value before using the output, but setting the output to the original text seems like it would be least surprising in case fixed isn't checked.)

IanVS · 2017-06-19T12:20:49Z

Thanks for the review and great suggestions, @not-an-aardvark. I'll make your requested changes within the next few days.

eslintbot · 2017-06-25T03:42:25Z

LGTM

IanVS · 2017-06-25T03:43:59Z

lib/util/source-code-fixer.js

-            const fix = problem.fix;
-            const start = fix.range[0];
-            const end = fix.range[1];
+            if (typeof shouldFix !== "function" || shouldFix(problem)) {


Note, we are only checking "truthiness" here. I think that's okay, but the other option would be to check for an explicit true return value.

IanVS · 2017-06-25T03:44:32Z

Thanks again, @not-an-aardvark. I believe I've addressed your comments.

not-an-aardvark

LGTM, thanks!

eslintbot · 2017-06-28T03:12:16Z

LGTM

This way, the code that uses this doesn’t need to necessarily check the value of `fixed`.

If we’ve gotten to this point, at least one fix will have been applied.

Meaning, that they cannot access the `this` value of SourceCodeFixer.

This is to verify that the problem can be used to return true or false conditionally, and that eslint will only apply fixes when true is returned.

eslintbot · 2017-06-28T04:54:29Z

LGTM

This is to account for #8809

kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint labels Jun 14, 2017

IanVS changed the title ~~Allow passing a function as fix option (fixes #8039)~~ New: Allow passing a function as fix option (fixes #8039) Jun 15, 2017

IanVS force-pushed the issue8039-fix-function branch from 96f7558 to c2f0916 Compare June 15, 2017 04:02

ilyavolodin approved these changes Jun 19, 2017

View reviewed changes

not-an-aardvark self-requested a review June 19, 2017 02:49

not-an-aardvark suggested changes Jun 19, 2017

View reviewed changes

IanVS commented Jun 25, 2017

View reviewed changes

not-an-aardvark approved these changes Jun 25, 2017

View reviewed changes

platinumazure approved these changes Jun 28, 2017

View reviewed changes

IanVS force-pushed the issue8039-fix-function branch from 73f556d to d06f84a Compare June 28, 2017 03:12

IanVS added 7 commits June 27, 2017 23:13

New: Allow passing a function as fix option (fixes #8039)

6edc672

Pass fix in options, instead of separate arg

24d62a2

Simplify conditional logic

d8fa64e

Return source code if shouldFix is false

e0c3035

This way, the code that uses this doesn’t need to necessarily check the value of `fixed`.

Clarify that fixesWereApplied is always true here

e7b5f4a

If we’ve gotten to this point, at least one fix will have been applied.

Add a test to ensure that fix functions are pure

4966811

Meaning, that they cannot access the `this` value of SourceCodeFixer.

Add test with conditional shouldFix

d06f84a

This is to verify that the problem can be used to return true or false conditionally, and that eslint will only apply fixes when true is returned.

Account for options not being provided

bc94f81

This is to account for #8809

not-an-aardvark merged commit c693be5 into master Jun 29, 2017

not-an-aardvark deleted the issue8039-fix-function branch June 29, 2017 02:26

platinumazure mentioned this pull request Jun 30, 2017

Confusion around how --quiet and --fix should behave together #8675

Closed

HomeZenBot mentioned this pull request Jul 10, 2017

⬆️ eslint @ 4.2.0 homezen/eslint-config-homezen#43

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Allow passing a function as `fix` option (fixes #8039) #8730

New: Allow passing a function as `fix` option (fixes #8039) #8730

IanVS commented Jun 14, 2017

eslintbot commented Jun 14, 2017

mention-bot commented Jun 14, 2017

eslintbot commented Jun 15, 2017

IanVS commented Jun 15, 2017

not-an-aardvark left a comment

not-an-aardvark Jun 19, 2017

not-an-aardvark Jun 19, 2017

not-an-aardvark Jun 19, 2017

not-an-aardvark Jun 19, 2017

IanVS Jun 19, 2017

not-an-aardvark Jun 19, 2017

not-an-aardvark Jun 19, 2017

IanVS commented Jun 19, 2017

eslintbot commented Jun 25, 2017

IanVS Jun 25, 2017

IanVS commented Jun 25, 2017

not-an-aardvark left a comment

eslintbot commented Jun 28, 2017

eslintbot commented Jun 28, 2017

New: Allow passing a function as fix option (fixes #8039) #8730

New: Allow passing a function as fix option (fixes #8039) #8730

Conversation

IanVS commented Jun 14, 2017

eslintbot commented Jun 14, 2017

mention-bot commented Jun 14, 2017

eslintbot commented Jun 15, 2017

IanVS commented Jun 15, 2017

not-an-aardvark 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

IanVS commented Jun 19, 2017

eslintbot commented Jun 25, 2017

Choose a reason for hiding this comment

IanVS commented Jun 25, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

eslintbot commented Jun 28, 2017

eslintbot commented Jun 28, 2017

New: Allow passing a function as `fix` option (fixes #8039) #8730

New: Allow passing a function as `fix` option (fixes #8039) #8730