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

Improved linting & formatting support for developers #2425

Open
robyngit opened this issue May 29, 2024 · 2 comments
Open

Improved linting & formatting support for developers #2425

robyngit opened this issue May 29, 2024 · 2 comments
Labels
documentation Creating and enhancing app documentation enhancement good first issue A good place to start contributing to MetacatUI! Priority: Low

Comments

@robyngit
Copy link
Member

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:

  • Add instructions on how to integrate prettier & eslint with common IDEs in CONTRIBUTING.md, e.g. from @ianguerin:

    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,
    ...

  • Consider including shareable vscode config in the repo

  • Improve GitHub actions to include job summaries and better PR comments *

  • Pre-commit hooks (with warnings)

  • Correct format automatically with prettier (e.g. use github actions to commit formatting corrections when needed)


* 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]
- 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 robyngit added enhancement Priority: Low documentation Creating and enhancing app documentation good first issue A good place to start contributing to MetacatUI! labels May 29, 2024
@robyngit
Copy link
Member Author

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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Creating and enhancing app documentation enhancement good first issue A good place to start contributing to MetacatUI! Priority: Low
Projects
None yet
Development

No branches or pull requests

2 participants