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

Fix the failing linting step #723

Merged
merged 14 commits into from
May 21, 2024
Merged

Fix the failing linting step #723

merged 14 commits into from
May 21, 2024

Conversation

ingeniumed
Copy link
Contributor

Description

As the next step for modernization, this PR:

  • Updates eslint packages, and adds in the rules from @automattic/eslint-plugin-wpvip as a starting point. There are a lot of rules being switched off in order to get the linting to pass. This will slowly be fixed as the underlying code is updated as well as the node/installed packages.
  • Runs linting on all the JS files and updates the build as a result
  • Removes the wordpress prettier packages as the above eslint plugin has that built in.

@ingeniumed
Copy link
Contributor Author

Note: The failing test is a flaky test that passes when it's re-run

@ingeniumed ingeniumed self-assigned this May 21, 2024
Base automatically changed from fix/gh-actions to develop May 21, 2024 17:21
@alecgeatches
Copy link
Contributor

alecgeatches commented May 21, 2024

@ingeniumed This looks good! I noticed some libraryish files in common/js have been formatted (probably through --fix), but it'd probably make more sense to revert and ignore those entirely.

I'm looking into that flaky test failure now.

jest: true,
},
rules: {
"no-prototype-builtins": 0,
Copy link
Contributor

@alecgeatches alecgeatches May 21, 2024

Choose a reason for hiding this comment

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

@ingeniumed Do you think any of these rules are more important than the others and we can apply them now? Or maybe we can make issues to address the more important exceptions (no-eval) soon. I get ignoring huge refactorings like valid-jsdoc, but it'd be great if low-hanging code quality and safety rules would be implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid adding more scope to this PR by resolving those, that's really why I avoided adding it in here. I also think as we build more features into this plugin, we are likely to change/upgrade that code (which might happen anyways thanks to the package modernizations)

@ingeniumed
Copy link
Contributor Author

@ingeniumed This looks good! I noticed some libraryish files in common/js have been formatted (probably through --fix), but it'd probably make more sense to revert and ignore those entirely.

I'm looking into that flaky test failure now.

Good point, I'll revert that. In another PR it might be worth seeing if there's a built in equivalent of that library now, as that could allow us to drop that code.

Copy link
Contributor

@alecgeatches alecgeatches left a comment

Choose a reason for hiding this comment

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

👍

@ingeniumed ingeniumed merged commit a884b37 into develop May 21, 2024
10 checks passed
@ingeniumed ingeniumed deleted the fix/lint-step branch May 21, 2024 22:34
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.

2 participants