robyngit
commented
1 month ago Initial results from first prettier run
Code style issues found in 437 files
Closed robyngit closed 1 month ago
robyngit
commented
1 month ago Code style issues found in 437 files
robyngit
commented
1 month ago 1871 problems (1871 errors, 0 warnings), over 228 files
Note, these results are from a run in the src dir only, excluding src/components. We may also want to apply the same or other rules to /tests/ and /docs/
robyngit
commented
1 month ago I've been experimenting with using a publicly available config of linting rules, airbnb-base, which seem to be a popular set of standards to adopt. However, this leads to 30470 errors to fix within the src dir only, with 17659 that can be automatically corrected. These automatic corrections break parts of the site, like the new data catalog view, and it's difficult to track down why.
My plan is to revert back to the more simple eslint config, without airbnb-base, and start adopting those rules more gradually.
yvonnesjy
commented
1 month ago 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
robyngit
commented
1 month ago 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
commented
1 month ago @yvonnesjy @ianguerin @ianguerin @rushirajnenuji - this PR is ready for review!
To test the new formatting & linting tools in this PR:
gh pr checkout 2412npm install to add new dev dependenciesnpm run format to have prettier auto-format code (npm run format-check to see errors without making changes)npm run lint to see the linting errors (!!!)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 https://github.com/NCEAS/metacatui/issues/2096#issuecomment-2073387618 for details.
ianguerin
commented
1 month ago Wow the automated GitHub comments are going to be a huge improvement for code review experience! Installation worked for me. I was able to get the ESLint findings to show up in VSCode by installing the ESLint extension by Microsoft, and adding the following lines to my User Settings JSON file:
...
"eslint.options": { "overrideConfigFile": "eslint.config.mjs" },
"eslint.experimental.useFlatConfig": true,
...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!
robyngit
commented
1 month ago 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:
robyngit
commented
1 month ago @yvonnesjy thank you so much for reviewing so thoroughly!
typos in existing code might cause the formatter to misinterpret... 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
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
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