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

Proposal: add prettier config #26

Merged
merged 4 commits into from
Mar 29, 2024
Merged

Conversation

Totto16
Copy link
Collaborator

@Totto16 Totto16 commented Mar 28, 2024

Proposal: add prettier config + git hook for checking the formatting

While working with this repo and submitting types to it, I used the autoformat feature of the IDE, I use prettier as my default to .ts / .d.ts / js files, so it used that. But since there is no local prettier config, it used my global one, since I prefer tabs vs spaces and no ; at the end, it was hugely different from the current codebase, but if you contribute to a codebase, it should be the same formatting. Atm the style is presumably the one, from the main contributor's global prettier config. So adding an explicit prettier config seems a good idea, I also added the git hook to check it before committing, so that (at least locally atm) the style is checked on each PR. For the future we can add a CI job, that checks the formatting on each PR, so that the style is coherent for the entire repo.

The current values are just my guess on them, and seem likely to match the current one used implicitly on most of the codebase, but feel free to suggest changes there, since this aims only to set a standard, the one that is used, is not that important (for me).

I didn't run the formatting yet (and tricked the commit hook manually in not complaining about it 😂 ), since it changes some files, we should do that after we set on a standard 👍🏼

@JumpLink
Copy link
Collaborator

@Totto16 Thank you! Can you please run yarn dlx @yarnpkg/sdks vscode? I think that would still be necessary for it to work with yarn

@swsnr
Copy link
Collaborator

swsnr commented Mar 28, 2024

Do you really need to commit these yarn sdk files? I thought these get created automatically on yarn install?

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 28, 2024

There were already committed files in these folders (and If they shouldn't be included, a .gitignore entry would be there, as far as I would do it), so I committed them too, I didn't work with yarn sdks before, so I don't know

Edit: Oh there is an entry in the .gitignore that explicitly states, to include them:

.yarn/*
!.yarn/patches
!.yarn/plugins
!.yarn/releases
!.yarn/sdks
!.yarn/versions

and a helpfull link: https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored

@JumpLink
Copy link
Collaborator

JumpLink commented Mar 28, 2024

According to yarn, it is recommended but not absolutely necessary: https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored

I agree with both, but if we ignore it then it should be written in the README.md that a yarn dlx @yarnpkg/sdks <your-favourite-editor> needs to be executed

@swsnr
Copy link
Collaborator

swsnr commented Mar 28, 2024

Ah okay, thanks for the explanation, let's keep them then. I must admit that I'm only superficially familiar with yarn 😇

@swsnr
Copy link
Collaborator

swsnr commented Mar 28, 2024

@Totto16 @JumpLink Shouldn't the ci.yaml workflow get a job to run prettier:check, to flag format errors on CI?

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 28, 2024

@Totto16 @JumpLink Shouldn't the ci.yaml workflow get a job to run prettier:check, to flag format errors on CI?

Yes I proposed that above, I will add this, before merging. I didn't do that yet, since that might create issues with PR's that were in the final phase, those are now merged and I see now problem in adding that 👍🏼

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 28, 2024

So as far as I understand, the given config is OK, so I'll format it and commit the changes 👍🏼

@Totto16 Totto16 requested a review from JumpLink March 28, 2024 21:28
@JumpLink JumpLink merged commit 0be3416 into gjsify:main Mar 29, 2024
1 check passed
@Totto16 Totto16 deleted the add-prettier-rc branch March 29, 2024 14:01
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