Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support autofix with eslint #79

Merged
merged 2 commits into from
Jun 21, 2021

Conversation

RainNapper
Copy link
Contributor

@RainNapper RainNapper commented Jun 18, 2021

Pasted from #80

When there are autofixable lint errors detected by eslint, the order of operations is:

  1. arc diff
  2. if there are uncommited changes, amend or commit
  3. arc lint, which autofixes and returns successful
  4. autofixed changes are not committed nor staged
  5. diff is pushed to phabricator without changes

This means that arc diff fails to properly apply lint changes before creating a diff.

See messages for a file provided by eslint@6.8.0

        "messages": [
            {
                "ruleId": "prettier/prettier",
                "severity": 2,
                "message": "Replace `(flow.component·&&·flow.component.archived)` with `flow.component·&&·flow.component.archived`",
                "line": 61,
                "column": 10,
                "nodeType": null,
                "messageId": "replace",
                "endLine": 61,
                "endColumn": 53,
                "fix": {
                    "range": [
                        1462,
                        1505
                    ],
                    "text": "flow.component && flow.component.archived"
                }
            },
            {
                "ruleId": "prettier/prettier",
                "severity": 2,
                "message": "Insert `(⏎··········`",
                "line": 65,
                "column": 13,
                "nodeType": null,
                "messageId": "insert",
                "endLine": 65,
                "endColumn": 13,
                "fix": {
                    "range": [
                        1675,
                        1675
                    ],
                    "text": "(\n          "
                }
            },
            {
                "ruleId": "prettier/prettier",
                "severity": 2,
                "message": "Replace `⏎⏎⏎········` with `········)`",
                "line": 66,
                "column": 1,
                "nodeType": null,
                "messageId": "replace",
                "endLine": 69,
                "endColumn": 9,
                "fix": {
                    "range": [
                        1685,
                        1696
                    ],
                    "text": "        )"
                }
            }
        ],

Summary of changes:

  1. Parse the fix fields in each message, and build ArcanistLintMessage with originalText and replacementText populated, along with a severity of ArcanistLintSeverity::SEVERITY_AUTOFIX. This means that the user will receive a prompt asking if they'd like to apply the fixes.
  2. Remove the eslint.fix flag because it likely causes more confusion that it helps.
  3. Use the existing eslint.fix flag to toggle this logic for backwards compatibility and as a way of gating these changes.

@RainNapper RainNapper requested a review from jparise as a code owner June 18, 2021 02:14
Comment on lines 87 to 90
'eslint.fix' => array(
'type' => 'optional bool',
'help' => pht('Specify whether eslint should autofix issues. (https://eslint.org/docs/user-guide/command-line-interface#fixing-problems)'),
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping this flag is backwards-incompatible. Is there a nice way to preserve it for people who are relying on it? Perhaps the new behavior you've added could be conditional based on a boolean attribute that's set by this flag.

Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jparise jparise changed the title Support autofix with eslint, and remove fix flag Support autofix with eslint Jun 21, 2021
@jparise jparise merged commit acc7c60 into pinterest:master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants