Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Format code with Prettier #16

Closed
wants to merge 3 commits into from

Conversation

singingwolfboy
Copy link

@singingwolfboy singingwolfboy commented Apr 9, 2020

Fixes #18. See: https://prettier.io

@bryanmacfarlane
Copy link
Member

bryanmacfarlane commented Apr 10, 2020

Thank you for your interest in this project.

Per the contribution guide, there should be an issue created before proceeding to code changes.

https://github.com/actions/http-client/blob/master/README.md#contributing

If we are going to consider auto formatting code, I would want to have a discussion with core contributors on the formatting rules we want; preferably consistent with our other repos. We would also want to be automatically applied and / or enforced as part of the CI with the same set of rules and prettier versions.

The PR is also hard to review with most lines changed and many for non formatting reasons ( line endings etc )

@singingwolfboy
Copy link
Author

Per the contribution guide, there should be an issue created before proceeding to code changes.

Done! The issue is #18.

If we are going to consider auto formatting code, I would want to have a discussion with core contributors on the formatting rules we want; preferably consistent with our other repos. We would also want to be automatically applied and / or enforced as part of the CI with the same set of rules and prettier versions.

Sure. I just did a quick overview of your other repos, and saw that Prettier is already being used in most of the Javascript/Typescript repos under https://github.com/actions. I'm all in favor of bringing them all up to the latest version of Prettier and enforcing that as part of CI.

The PR is also hard to review with most lines changed and many for non formatting reasons ( line endings etc )

There's nothing to be done about that. By design, Prettier controls all aspects of code formatting, and is barely configurable at all. This makes it much simpler to use. This commit was made by running the prettier tool over the codebase and committing the changes it made, without further modification. If it helps for reviewing purposes, it make be interesting to know that Prettier works by parsing the existing code to an AST, reformatting, re-parsing the formatted code to an AST, and comparing the two ASTs to be sure they are identical. That means there should be zero chance that Prettier's changes result in a change to the behavior of the project, since the behavior is determined by the AST.

@bryanmacfarlane bryanmacfarlane mentioned this pull request Apr 10, 2020
@bryanmacfarlane
Copy link
Member

There's nothing to be done about that (large diff)

As pointed out in #20, making the rules part of the PR allows for a common understanding and review independent of a large diff changing all lines. I would also prefer a separate commit by a core contributor with the rules applied (changing all lines) so a contributor does not have to review all lines in the whole project comparing against the rules. Since this PR didn't have the rules or the workflow file but instead had the portion we would want a core contributor to push, the PR isn't really applicable. Going forward, creating an issue per the contributors guidelines with a discussions would have helped. Hope that clarifies.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code should be formatted using prettier
2 participants