NCEAS / metacatui

MetacatUI: A client-side web interface for DataONE data repositories
https://nceas.github.io/metacatui
Apache License 2.0
41 stars 27 forks source link

Improved linting & formatting support for developers #2425

Open robyngit opened 4 months ago

robyngit commented 4 months ago

Linting & formatting is implemented in #2412. GitHub actions were added and the CONTRIBUTING docs updated to help developers adopt the code standards, however, there are more improvements we could make in this regard.

Ideas for potential improvements:


* I started working on improved GitHub actions during #2412, but opted to keep it simpler for the first release instead. Here is the WIP code in case it can be put to use:

💻 Github actions snippet to create markdown summary & custom PR comment [WIP] ```yaml - name: Set env vars for subsequent steps env: OVERALL_OUTCOME: ${{ job.status }} PASS_MD: '✅ PASS' FAIL_MD: '❌ FAIL' FORMATTING_OUT: ${{ steps.prettier_check.outcome }} LINTING_OUT: ${{ steps.eslint_check.outcome }} UNIT_OUT: ${{ steps.test.outcome }} JSDOC_OUT: ${{ steps.jsdoc-dry-run.outcome }} run: | echo "OVERALL_OUTCOME=${{ env.OVERALL_OUTCOME }}" >> $GITHUB_ENV echo "PASS_MD=${{ env.PASS_MD }}" >> $GITHUB_ENV echo "FAIL_MD=${{ env.FAIL_MD }}" >> $GITHUB_ENV echo "FORMATTING_OUT=${{ env.FORMATTING_OUT }}" >> $GITHUB_ENV echo "LINTING_OUT=${{ env.LINTING_OUT }}" >> $GITHUB_ENV echo "UNIT_OUT=${{ env.UNIT_OUT }}" >> $GITHUB_ENV echo "JSDOC_OUT=${{ env.JSDOC_OUT }}" >> $GITHUB_ENV echo "FORMATTING_OUT_MD=${{ env.FORMATTING_OUT == 'success' && env.PASS_MD || env.FAIL_MD }}" >> $GITHUB_ENV echo "LINTING_OUT_MD=${{ env.LINTING_OUT == 'success' && env.PASS_MD || env.FAIL_MD }}" >> $GITHUB_ENV echo "UNIT_OUT_MD=${{ env.UNIT_OUT == 'success' && env.PASS_MD || env.FAIL_MD }}" >> $GITHUB_ENV echo "JSDOC_OUT_MD=${{ env.JSDOC_OUT == 'success' && env.PASS_MD || env.FAIL_MD }}" >> $GITHUB_ENV - name: Render workflow summary template id: summary-template uses: chuhlomin/render-template@v1.4 with: template: .github/WORKFLOW_TEMPLATE/format-test-lint-summary.md vars: | formatting_out_md: ${{ env.FORMATTING_OUT_MD }} linting_out_md: ${{ env.LINTING_OUT_MD }} unit_out_md: ${{ env.UNIT_OUT_MD }} jsdoc_out_md: ${{ env.JSDOC_OUT_MD }} - name: Write summary to step summary run: echo "${{ steps.summary-template.outputs.result }}" >> $GITHUB_STEP_SUMMARY - name: Choose PR comment template based on outcome id: choose-template env: GUIDANCE_TEMPLATE: .github/WORKFLOW_TEMPLATE/pr-comment-guidance.md CONFIRMATION_TEMPLATE: .github/WORKFLOW_TEMPLATE/pr-comment-confirmation.md run: | if [ ${{ env.OVERALL_OUTCOME }} == 'success' ]; then echo "PR_COMMENT_TEMPLATE=${{ env.CONFIRMATION_TEMPLATE }}" >> $GITHUB_ENV else echo "PR_COMMENT_TEMPLATE=${{ env.GUIDANCE_TEMPLATE }}" >> $GITHUB_ENV fi - name: Render PR comment template id: guidance-template uses: chuhlomin/render-template@v1.4 with: template: ${{ env.PR_COMMENT_TEMPLATE }} vars: | formatting_out_md: ${{ env.FORMATTING_OUT_MD }} linting_out_md: ${{ env.LINTING_OUT_MD }} unit_out_md: ${{ env.UNIT_OUT_MD }} jsdoc_out_md: ${{ env.JSDOC_OUT_MD }} - name: Create comment uses: peter-evans/create-or-update-comment@v4 with: issue-number: ${{ github.event.pull_request.number }} body: ${{ steps.guidance-template.outputs.result }} token: ${{ secrets.GITHUB_TOKEN }} ```

WIP Markdown templates referenced by the above GitHub Actions snippet:

robyngit commented 2 months ago

From a conversation with an onboarding dev about linting setup. We could add some of this to the contributing docs:

Many of the linting issues can't be autofixed. And in fact, when we try to auto fix what can be automatically fixed in the codebase, things break. So that's why we are implementing these linting rules gradually.

I would recommend installing some extensions in VScode to help

Name: ESLint
Id: dbaeumer.vscode-eslint
Description: Integrates ESLint JavaScript into VS Code.
Version: 3.0.10
Publisher: Microsoft
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint

and

Name: toggle-eslint
Id: zentus.toggle-eslint
Description: Toggle ESLint
Version: 0.0.3
Publisher: zentus
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=zentus.toggle-eslint

For the ESLint extension, you may also need to add some settings to your VSCode set up.... To edit settings. json , utilize the Command Palette Cmd + Shift + P for macOS or Ctrl + Shift + P for Windows and then type “Preferences: Open Settings (JSON)”

I am using the following....

"eslint.validate": [
"javascript"
],
"eslint.experimental.useFlatConfig": true,
"eslint.execArgv": null,
"eslint.enable": true,

This allows you to browse through the linting errors and sometimes even see suggestions on how to fix

For a PR, the convention at the moment is to at least not having linting errors in any lines of code that you add or modify. Fixing the entire file is not necessary. If you do go through and fix linting errors in a file, outside of the code you've modified, then that should be done in a separate commit.

alonakos commented 1 month ago

During onboarding, I attempted to install the extensions and set up eslint, but I still have issues when committing PR and tons of enlist errors locally. Maybe we could prioritize looking into adding eslint into package.json as part of the codebase or some other way to make it a standardized process? Alternatively, could we expand the Contributing section specifically for the enlist part to make the processes smoother?