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

Is ktfmt used on itself? #464

Closed
grodin opened this issue May 7, 2024 · 5 comments
Closed

Is ktfmt used on itself? #464

grodin opened this issue May 7, 2024 · 5 comments

Comments

@grodin
Copy link

grodin commented May 7, 2024

Starting work on #391, I realised there's no editorconfig and checking the CI setup, there's no formatting check step there either.

My main concern is to avoid misformatting the work I'm about to do, but I thought it was a potential source of contributor friction so worth raising as an issue.

Happy to help on this too if wanted.

@hick209
Copy link
Contributor

hick209 commented May 7, 2024

We do indeed run ktfmt on ktfmt code (🤯).
Feel free to improve the process (adding editorconfig file), but we also do run the formatting to the code when we import your PR internally so don't worry about misformatting things.

@nreid260
Copy link
Contributor

In the past, I've been told not to format before sending a PR. The reasoning is that, the PR is probably changing the formatter behaviour, which creates a disproportionate amount of noise. Reformatting is left as a followup.

@grodin
Copy link
Author

grodin commented May 20, 2024

In the past, I've been told not to format before sending a PR. The reasoning is that, the PR is probably changing the formatter behaviour, which creates a disproportionate amount of noise. Reformatting is left as a followup.

That's a very good point. It's probably better not to include an editorconfig then. It doesn't matter too much if FB are formatting the code when they merge internally anyway.

Probably worth mentioning all of the above in CONTRIBUTING.md.

@hick209
Copy link
Contributor

hick209 commented May 20, 2024

Good point @grodin! We should change that

@hick209
Copy link
Contributor

hick209 commented Jun 5, 2024

@grodin do you want to submit a PR for that?

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 a pull request may close this issue.

3 participants