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

Upgrade eslint-config-airbnb #12760

Merged
merged 2 commits into from
May 28, 2020
Merged

Conversation

jhonnyoddball
Copy link
Contributor

Description

This PR will upgrade eslint-plugin-jsx-a11y and eslint to the latest version

There are a lot of new rules and modifications to existing ones. They have been temporarily disabled until the update is complete.

Most of these rules will be evaluated and errors will be fixed before turn them on.

Testing done

Locally

Acceptance criteria

  • No errors caused

Definition of done

  • Events are logged appropriately
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@jhonnyoddball jhonnyoddball requested review from bkjohnson and a team May 21, 2020 18:26
@va-bot va-bot temporarily deployed to vetsgov-pr-12760 May 21, 2020 18:27 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/upgrade-eslint-config-airbnb May 21, 2020 18:50 Inactive
Comment on lines +32 to +35
// "no-unused-vars": [
// 2,
// { "args": "after-used", "argsIgnorePattern": "^_", "vars": "local" }
// ],
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the errors that are preventing this rule from staying on?

Copy link
Contributor Author

@jhonnyoddball jhonnyoddball May 22, 2020

Choose a reason for hiding this comment

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

This rule was commented out here because it was added to the audit on line 122. We will activate it once all errors are fixed.

Screen Shot 2020-05-22 at 9 46 39 AM

@@ -27,12 +27,12 @@
"rules": {
/* || Eslint main rules || */
"camelcase": [2, { "properties": "always" }], // Override airbnb style.
"func-names": 2,
// "func-names": 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule as well. Why are we changing it?

Copy link
Contributor Author

@jhonnyoddball jhonnyoddball May 22, 2020

Choose a reason for hiding this comment

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

This rule was commented out here because it was added to the audit on line 119. We will activate it once all errors are fixed.

Screen Shot 2020-05-22 at 9 52 28 AM

@jhonnyoddball
Copy link
Contributor Author

I added the current errors to each rule as a comment.

@rianfowler rianfowler requested a review from 1Copenut May 27, 2020 15:07
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

LGTM. I ran $ yarn lint:js locally and came up with zero errors. I'll be interested to see how this evolves as rules are added back.

@jhonnyoddball
Copy link
Contributor Author

I'll be working on turning some of these on soon. I noticed there are some jsx-a11y additions with not too many errors that we can turn on.

@va-bot va-bot temporarily deployed to vetsgov-pr-12760 May 27, 2020 18:07 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/upgrade-eslint-config-airbnb May 27, 2020 18:40 Inactive
@jhonnyoddball jhonnyoddball merged commit 4e03bc8 into master May 28, 2020
@jhonnyoddball jhonnyoddball deleted the upgrade-eslint-config-airbnb branch May 28, 2020 12:43
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.

5 participants