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: Apply prettier on the whole project files #313

Merged
merged 15 commits into from
Dec 27, 2023

Conversation

zoontek
Copy link
Contributor

@zoontek zoontek commented Dec 25, 2023

📜 Description

This is a proposal. When I'm working on the repository, I notice that the formatter (prettier) update some part of the files even if I don't changed them. This occurs because some files are not formatted with it, this PR add a new npm command (format) and run it.

I know that this is a lot of changes (especially in the docs markdown files), feels free to close the PR if you don't want to deal with this.

💡 Motivation and Context

This simplifies collaboration on the project, can even been run on commit using lint-staged.

Copy link

argos-ci bot commented Dec 25, 2023

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) 🧿 Changes detected (Review) 17 changes Dec 27, 2023, 8:57 PM

@kirillzyusko
Copy link
Owner

Thank you @zoontek for your PR
I knew that only code in src was properly formatted, but somehow ignored it 🙈

Overall I like the idea of auto-formatting and definitely would like to merge this PR. But I have some questions:

  • could you rebase to latest changes and apply prettier again? I just prepared a new release so some new files were added recently.
  • could you add GH action that will verify that all code is properly formatted? (I like the usage of git-hooks, but some external contributors may push/commit with --no-verify flag, so I'd like to have an independent verifier in CI)

@zoontek
Copy link
Contributor Author

zoontek commented Dec 26, 2023

could you add GH action that will verify that all code is properly formatted? (I like the usage of git-hooks, but some external contributors may push/commit with --no-verify flag, so I'd like to have an independent verifier in CI)

There's already eslint-plugin-prettier on the project, maybe we should extend the linting to the whole project? (by removing docs, example and FabricExample from ignorePatterns)

Screenshot 2023-12-26 at 20 43 46

There's currently a lot of issues with the import/order rule if you do this, but we can run lint with --fix to clean that. I did it in this commit: 1d6f159

This makes a lot of changes, but rebasing is really easy: never pick the changes from this branch in code, run yarn format and yarn lint --fix once the rebase is done.

@zoontek
Copy link
Contributor Author

zoontek commented Dec 26, 2023

  • I updated @react-native/eslint-config to the latest version, aligned dependencies to match the config deps (eslint-config-prettier, eslint-plugin-jest and eslint-plugin-prettier)
  • I had to bump actions to use node v18.x as v16.x is deprecated with this version of the eslint config
  • I updated the prettier config to use singleQuote: false, tabWidth: 2, useTabs: false (the defaults in prettier v2) and trailingComma: "all" (the default in prettier v3)
  • I fixed the few linting errors which were not import/order errors

@kirillzyusko
Copy link
Owner

maybe we should extend the linting to the whole project? (by removing docs, example and FabricExample from ignorePatterns)

@zoontek yes, that's definitely a good point (and I've seen you already did that 🎉 ). The reason why I want to have a prettier as CI job is because eslint is scanning only JS/TS files, but I see that prettier also fixes problems in .md/.yml and other files (which is great).

So I think having a prettier on CI would be good, do you agree?

I'm happy to merge all these changes (though I haven't reviewed latest changes yet), but want to have prettier job in place to assure that all new code will be properly formatted 🙂

@zoontek
Copy link
Contributor Author

zoontek commented Dec 27, 2023

@kirillzyusko I added a "Check formatting" step, but in the verify-js.yml workflow, even if it's not only for JS.

It's a bit of a duplicate with the eslint plugin, but it's allow formatting issues reporting in dev when the dev has a an editor with eslint configured.

@kirillzyusko
Copy link
Owner

Awesome @zoontek

I'll review everything again today evening (and maybe do some other fixes on my own, do not want to disturb you once again) and then I'll merge this PR 😎

Thank you again for the contribution!

@kirillzyusko kirillzyusko merged commit b6d2ca8 into kirillzyusko:main Dec 27, 2023
22 of 25 checks passed
@zoontek zoontek deleted the apply-prettier branch December 27, 2023 22:27
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.

2 participants