Apply linting & formatting globally #2412
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robyngit
requested review from
ianguerin,
yvonnesjy,
iannesbitt and
rushirajnenuji
Initial results from first prettier runCode style issues found in 437 files Click to see the list of files formatted |
robyngit
force-pushed
the
feature-2096-linting-formatting
branch
from
a51d918
to
f33bf9e
Compare
ianguerin
approved these changes
May 13, 2024
Initial ESlint report1871 problems (1871 errors, 0 warnings), over 228 files Note, these results are from a run in the |
robyngit
force-pushed
the
feature-2096-linting-formatting
branch
from
e76bafd
to
21c22b7
Compare
|
I've been experimenting with using a publicly available config of linting rules, My plan is to revert back to the more simple eslint config, without Here is the config that included airbnb and mostly worked with our set upimport globals from "globals";
import pluginJs from "@eslint/js";
import { FlatCompat } from "@eslint/eslintrc";
import path from "path";
import prettierConfig from "eslint-config-prettier";
import requirejs from "eslint-plugin-requirejs";
import { fileURLToPath } from "url";
// This chunk needed to use older style shared eslint configs, e.g. airbnb-base
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
const compat = new FlatCompat({
baseDirectory: __dirname
});
export default [
// This object must contain no other properties than ignores in order for it
// to apply to all config objects in this array
{ ignores: ["src/components/"] },
// Use the recommended config for JS files
pluginJs.configs.recommended,
// Use the popular airbnb-base config, but extend it with our own rules
...compat.extends("airbnb-base"),
// Lint all the main MetacatUI files
{
name: "lint-views-models-and-collections",
files: ["src/**/*.js"],
plugins: {
requirejs
},
languageOptions: {
sourceType: "commonjs",
ecmaVersion: 2020,
globals: {
...globals.browser,
...globals.amd,
MetacatUI: "readonly",
}
},
// Use requirejs.configs.recommended.rules and some of our own rules
rules: {
...requirejs.configs.recommended.rules,
"import/no-amd": "off", // We use AMD
"no-console": "off", // We allow console.log for now
"strict": ["error", "global"], // We use strict mode
"func-names": "off", // require and backbone need to use unnamed functions
}
},
// Allow the prettier plugin to override any conflicting rules
...[].concat(prettierConfig),
]; |
|
Is it possible to have lint only run on changed lines/files so we can fix as we go? I think incremental fixes might be more practical than incremental rules adoption |
|
thanks @yvonnesjy! 🧠⚡ That's exactly the strategy we landed on in a discussion with the broader dev team. So the rules will be more strict, and will include the airbnb set, but we'll just apply fixes as we touch each file. |
robyngit
force-pushed
the
feature-2096-linting-formatting
branch
2 times, most recently
from
7264677
to
0531eb0
Compare
Issue 2096
globally defined packages are specified in eslint config Issue #2096
robyngit
force-pushed
the
feature-2096-linting-formatting
branch
from
9825dc8
to
2d57d15
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Run workflow on PRs only to avoid duplicated actions - Combine all checks into one workflow Issue #2096
robyngit
force-pushed
the
feature-2096-linting-formatting
branch
from
2d57d15
to
92f65bc
Compare
- Reformat & cleanup the doc as well Issue #2096
robyngit
force-pushed
the
feature-2096-linting-formatting
branch
from
420bdeb
to
2284d19
Compare
|
@yvonnesjy @ianguerin @ianguerin @rushirajnenuji - this PR is ready for review! To test the new formatting & linting tools in this PR:
The entire code base has been reformatted to use Prettier code style; however new linting standards will need to be implemented gradually. The PR introduces new GitHub actions to check for formatting & linting errors on new Pull Requests. See demo with auto generated comments pointing out errors and suggesting fixes where possible. PR comments are only made where the linting errors apply to the lines of code that were changed. However, the GitHub action test will fail if there are linting errors in any part of the files changed. I think we can decide on a case-by-case basis whether to fix entire files or allow PRs through with some errors - a balance between improving our code quality and encouraging quick merging of new contributions. The contributing guide has also been updated with these changes and generally cleaned up a bit: https://github.com/NCEAS/metacatui/blob/2284d194c5f5f1db3a374c316fb10a00a12bccb1/CONTRIBUTING.md Looking for feedback on this strategy (👍🏻 or 👎🏻 ?) and also whether installing and using the new tools goes smoothly. Note: Once this PR is merged into develop, there will likely be merge conflicts with any current feature branches. You may want to apply prettier formatting before merging with develop, see #2096 (comment) for details. |
|
Wow the automated GitHub comments are going to be a huge improvement for code review experience! I was able to get Prettier formatting to work in VSCode by installing the Prettier extension by Prettier. I bound a keyboard shortcut to format the document. I also installed Pretter ESLint by Rebecca Vest, but I'm not sure which formatter is more useful yet. Thanks for working on this! |
yvonnesjy
reviewed
May 29, 2024
There was a problem hiding this comment.
Thanks Robyn for setting it up!! This is going to be so helpful🎉
I started marking files as reviewed and quickly realized 400+ files will be a long stretch. Is it possible to break down the PR into 1. lint config files, 2. smaller PR's of updated files? Maybe ~50 changed files in a PR will be doable. Or more if the changes are super simple (adding/removing blank spaces)
|
Thanks for going through this @yvonnesjy! I recently discovered GitHub provides a UI for viewing PR changes commit by commit. The second commit is the one that changes nearly every file in the code base. The fourth commit also changes a lot of files to remove a special comment at the top of the file. Neither changes anything functionally, so I don't think those changes need to be reviewed file by file. Here is a breakdown of each commit with links to the files changed in just those commits:
|
yvonnesjy
approved these changes
Jun 3, 2024
There was a problem hiding this comment.
I got through more than half of commit 2 before I had to admit that I won't be able to process 100k+ lines of changes. I think the diff are generally low risk, but without comprehensive testing, I don't have confidence that we're 100% bug free. For example, typos in existing code might cause the formatter to misinterpret.
We can try to merge the PR as is, but if we ever run into surprise bugs that require rollbacks, we might want to consider breaking this down into smaller verifiable PR's so that we don't need to get everything right in one go.
ianguerin
approved these changes
Jun 3, 2024
|
@yvonnesjy thank you so much for reviewing so thoroughly!
These are really good points and I suspect that we could run into some of these issues. I am going to move forward with the PR as-is so that we can get the formatting changes in place before too many merge conflicts arise with existing and WIP branches. I will definitely keep this strategy in mind for future PRs that change large amounts of code, and |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request will add and configure prettier and eslint to MetacatUI and apply linting and formatting to all relevant files. Note that this PR is currently in draft mode but I would open the discussion and allow for feedback on the changes sooner rather than later.
Because these changes will impact nearly the entire codebase, incorporating the updates will present a challenge for groups that have extended MetacatUI. There may be similar merge conflicts for us to handle on branches that we've forked off of develop prior to these changes being merged in.
To reduce merge conflict headaches for external groups, I propose that these changes are presented in single independent release, with a series of commits that can be merged individually, and including detailed instructions on how to incorporate the changes into a forked codebase.
Proposed Commits
Roughly, the series of commits would be as follows:
Instructions for Incorporating Changes
The instructions for incorporating these changes into a forked codebase would something like:
npm installto install prettiernpm run formatto apply prettier formatting to all files