mermaid-cli icon indicating copy to clipboard operation
mermaid-cli copied to clipboard

Lint using `standard`

Open aloisklink opened this issue 2 years ago • 3 comments

:bookmark_tabs: Summary

Lints this project using standard.

I've added running standard to the package.json#scripts#test (in https://github.com/mermaid-js/mermaid-cli/commit/87c83014a15f7cf7c2ab430cd1dc6b4fa07fefc6), since that's what it says in CONTRIBUTING.md:

https://github.com/mermaid-js/mermaid-cli/blob/6fa464418488ead911faa2537ed4e2c33fc267f1/CONTRIBUTING.md?plain=1#L23

I've also added a GitHub Actions CI action that runs yarn test automatically for future PRs in https://github.com/mermaid-js/mermaid-cli/commit/34017714315dd1577e16610e1669aab7e7b0d228.

Finally, I've fixed any errors that were NOT automatically fixed by standard --fix in https://github.com/mermaid-js/mermaid-cli/commit/e77b3c92a0b59d5cc4adb975441925c75081af40. This is to reduce the git diff and make this PR easier to review/less likely to cause merge conflicts. All other linting errors should be fixed by running standard --fix right before/after merging (example: https://github.com/aloisklink/mermaid-cli/commit/0b936615694aa3441e6ba2b35c4ba0e78e249aa6).

:straight_ruler: Design Decisions

To simplify the .github/workflows/lint.yml file, I've moved the prepublishOnly script into a prepare and prepack script, as npm/yarn will call those scripts automatically, when needed (https://github.com/mermaid-js/mermaid-cli/commit/2a0965e1aa06a84739a8de44e33128110602185f).

npm run prepare is recommended for building NPM packages, as it's called automatically before npm pack, npm publish, and npm install . (local) or npm install git+https:// (git dependency).

yarn run prepack is recommended for building yarn packages, as it's called before yarn pack, yarn publish, and automatically when installing from "raw" sources, like a git dependency.

I've also modified all the *.sh scripts to have a !/bin/sh shebang, since the Alpine Linux Docker image doesn't actually support bash. Only run-tests.sh required a very minor change to be /bin/sh compliant (https://github.com/mermaid-js/mermaid-cli/commit/b43c86c3b0ee7def0f017adde157dc33077b25b1).

:clipboard: Tasks

Make sure you

  • [x] :book: have read the contribution guidelines
    • Auto-fixable linting issues have not been fixed, to make the git diff easier to review. Please run yarn run lint-fix right before/after merging.
  • [x] :computer: have added unit/e2e tests (if appropriate)
  • [x] :bookmark: targeted master branch

aloisklink avatar Jun 13 '22 03:06 aloisklink

Thank you for another high quality PR. Please, resolve the conflicts and have a look at my comment.

MindaugasLaganeckas avatar Jun 14 '22 08:06 MindaugasLaganeckas

I've rebased my commits onto master to fix the conflict. (git merge didn't seem to do anything, since the project uses rebasing).

I can't find your comment. That may be my fault or GitHub UI's fault: it might have disappeared when I resolved the git conflicts.

I haven't yet run yarn standard --fix (fixes minor style errors automatically), to make this PR smaller and easier to review. Because of that, the linting CI is failing. Let me know if/when you want me to add that commit, or if you're happy to add it.

And thanks to you too for maintaining this super awesome tool :smile:

aloisklink avatar Jun 16 '22 22:06 aloisklink

I am busy at the office now. I will get back to you in a couple of weeks. I am sorry for the delay :) I will tag in the comment. 😄

MindaugasLaganeckas avatar Jun 22 '22 07:06 MindaugasLaganeckas

@aloisklink can you rebase your branch? I see a great value in this PR 😄

MindaugasLaganeckas avatar Aug 18 '22 12:08 MindaugasLaganeckas

@aloisklink can you rebase your branch? I see a great value in this PR :smile:

:rocket:

Rebased onto https://github.com/mermaid-js/mermaid-cli/commit/9fa8d397633d0265c22c63a345e834ec73d812cf (current master)

I've slightly modified this PR too, since some of my other PRs have made part of this PR outdated.

CI is failing because I haven't run yarn standard --fix yet, to make review easier. I can do it before merging, or you can do it after merging.

aloisklink avatar Aug 18 '22 18:08 aloisklink