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

Added prettier, lint-staged and husky. #139

Merged
merged 4 commits into from
Sep 7, 2023

Conversation

lukester1975
Copy link
Contributor

As suggested in #68.

Just a stab at a prettier config. (I personally don't understand the preference for 80 columns... my monitor is wider than I am tall, and I'm 6'5")

print-width 100 example: https://github.com/lukester1975/vscode-meson/compare/78052ee5f2c33fcdaa25b93698726b10d658a71d..1fbd08082f804f73366ba032005d3101005d9f1d
80: https://github.com/lukester1975/vscode-meson/compare/78052ee5f2c33fcdaa25b93698726b10d658a71d..856fbf0fa785f62117d4af8da0ad58baa0fc2645

Copy link
Contributor

@tristan957 tristan957 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a GitHub Action for using Prettier?

Here is an example: https://github.com/tristan957/libmerr/blob/master/.github/workflows/prettier.yaml

.husky/pre-commit Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
package.json Outdated
"dependencies": {}
"dependencies": {},
"prettier": {
"printWidth": 100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just finish the rest of the config.

Determine if we are using tabs or spaces, and choose it.

semi: true
singleQuote: false
quoteProps: as-needed
jsxSingleQuote: false
trailingCommas: all
bracketSpacing: true
bracketSameLine: false
arrowParens: true
wrapProse: always
endOfLine: lf

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces

@lukester1975
Copy link
Contributor Author

GH action - sure, will take a look at some point.

@dcbaker
Copy link
Member

dcbaker commented Apr 25, 2023

I haven't looked at anything but the initial MR message. I'd be happy with 100 or 120 column widths. I'd prefer not to go wider than that as I usually have two files opened side-by side. I think Meson itself is 100?

@tristan957
Copy link
Contributor

@lukester1975 any time to come back to this?

Run prettier as a pre-commit hook.
@lukester1975
Copy link
Contributor Author

I'm not a GH actions person but this appears to work based of yours @tristan957.

  • meson uses a line length of 120 by the looks of it (in .flake8), so did the same here.
  • Since there's an .editorconfig, I put the line length there as prettier will use that.

Not sure I see the point of specifying options with the default values: as soon as a new value is added, it's no longer complete anyway. Plus if a default ever changes, highlighting that fact and reviewing what is preferred is a good thing IMHO. Plus package.json is too long as it is...

.github/workflows/prettier.yml Outdated Show resolved Hide resolved
.github/workflows/prettier.yml Outdated Show resolved Hide resolved
.github/workflows/prettier.yml Outdated Show resolved Hide resolved
@@ -16,12 +16,12 @@ permissions:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping the reformatting a separate commit. Could you add one more commit to this PR that adds a .git-blame-ignore-revs file to the root of the repo? Similar to https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs. The only known entry would the commit where you reformatted everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already is one apparently, so I've added the commit. Will try to remember it on next rebase, unless this is getting merged soon...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ready to merge. Are you good to go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, if you are. Checks are still queued though...

@tristan957
Copy link
Contributor

Weird. I will watch it.

@tristan957 tristan957 merged commit e7d513c into mesonbuild:main Sep 7, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants