primer / primitives

Color, typography, and spacing primitives in json.
https://primer.style/primitives
MIT License
295 stars 45 forks source link

Format script is very unhelpful and not the same as lint #376

Closed lukasoppermann closed 1 year ago

lukasoppermann commented 1 year ago

Problem

While yarn lint passes, yarn format fails. This is often only noticed in the PR as I don't run format and lint. However the output is very unhelpful.

Expected behavior

  1. If yarn lint passes, yarn format should pass as well
  2. If either fails in a PR, the issues should be reported in the action or in the PR as a comment. If people add a PR via github they have no chance of knowing what is wrong.
rezrah commented 1 year ago

However the output is very unhelpful.

@lukasoppermann - can you please post an example of the output you found unhelpful?

If yarn lint passes, yarn format should pass as well

🤔 Is this expected behaviour? IMO they've usually always had different responsibilities, but we can merge the commands if you and others find that more useful.

If either fails in a PR, the issues should be reported in the action or in the PR as a comment.

AFAIK, we don't do this anywhere, but I kinda like it as it avoids you digging into the CI workflow to see what failed.

Another option to shift-left on this would be to introduce a pre-commit hook, that brings the error to your attention sooner.

lukasoppermann commented 1 year ago

Hey, so first this is the output

$ prettier --check '*/.{js,jsx,ts,tsx,md,mdx,css}' Checking formatting... [warn] .github/pull_request_template.md [warn] Code style issues found in the above file. Forgot to run Prettier? error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

But I would love to know what issues were found.

🤔 Is this expected behaviour? IMO they've usually always had different responsibilities, but we can merge the commands if you and others find that more useful.

I don't know maybe I am off here. I would think I need to run the linter and it tells me what is wrong. But isn't linting supposed to catch code style errors? So why would it not complain and format would than complain about code style issues?

Also we do have a prettier plugin in eslint.

Note: Did a quick google and I guess you are referring to behavior vs. style with lint vs. format, right? I think this is actually not really true in the wild, which is why this difference was not apparent to me.

However if we can somehow manage to provide one easy way for a user to do it, I am of course fine with the separate. Either with a pre-commit hook or (or both) with one npm script that combines both and shows what needs to be fixed.

Another option to shift-left on this would be to introduce a pre-commit hook, that brings the error to your attention sooner.

I would love a pre-commit hook as I always forget to run lint.

I think we should still show the errors to fix at least in the CI workflow, so that one can fix them when doing a PR from the github UI directly without an IDE.

lukasoppermann commented 1 year ago

Hey @rezrah maybe we could add the correct vs code settings for autoformat to .vscode/settings.json?

Would that be this?

{
  "typescript.tsdk": "node_modules/typescript/lib",
  "editor.defaultFormatter": "rvest.vs-code-prettier-eslint"
}