Skip to content

Streamline code formatting #7757

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

Closed
wants to merge 34 commits into from

Conversation

trumbitta
Copy link
Contributor

@trumbitta trumbitta commented Jan 21, 2022

Description

Why

  • I'm about to start working on fixing the linting problems the dashboard app logs, but ESLint and Prettier have some common gray area and it's better to start with consistently formatted code to have an easier time in handling this common area
  • Basically why Prettier exists: having an automated coding style which is readable, consistent, and not particularly prone to be argued upon: again, @loujaybee this improves DevX

What

  • Adds Prettier support and configurations
  • Format on save
  • Batch format everything Prettier can format to obtain a good starting point
  • Monorepo components / folders are batch-formatted in separate commits in case you prefer to accept this only for some of them

TODO

  • Get feedback, is this useful? (I think it's fundamental!)
  • Get feedback, do we want it everywhere Prettier can do its thing? (again, see the commits)
  • Add notes to README (which ones depends on the answer to the question just above) about this
  • Add a pre-commit hook, just in case. But this seems better done in a separate PR after / with ESLint because Prettier suggests doing it with https://github.com/okonet/lint-staged#configuration and lint-staged suggests stabilizing ESLint before configuring it
  • Get someone to try it with a Jetbrains IDE

How to test

Use, build, test Gitpod to see if this broke something (it shouldn't have, since Prettier only reformats code)

@roboquat
Copy link
Contributor

@trumbitta: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign akosyakov, alextugarev, corneliusludmann, csweichel after the PR has been reviewed.
You can assign the PR to them by writing /assign @akosyakov @alextugarev @corneliusludmann @csweichel in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat added team: IDE team: webapp Issue belongs to the WebApp team team: devx team: delivery Issue belongs to the self-hosted team team: workspace Issue belongs to the Workspace team size/XXL labels Jan 21, 2022
@Siddhant-K-code
Copy link
Member

We Can add small automation too, that will run the prettier script ( to format whole code) before every commit. This way whole code will be formatted well.

@gtsiolis
Copy link
Contributor

gtsiolis commented Jan 26, 2022

Thanks for giving this a go, @trumbitta! 💯

Files changed: 894

question: Could we break this down into smaller pieces or steps to make it easier and faster to review? Breaking down changes per component, starting with the bare minimum linting rules, or leaving markdown files out of the scope of these changes could help. 💭

Cross-linking relevant issue in #3841.

@trumbitta
Copy link
Contributor Author

trumbitta commented Jan 26, 2022

Thanks for giving this a go, @trumbitta! 💯

Files changed: 894

question: Could we break this down into smaller pieces or steps to make it easier and faster to review? Breaking down changes per component, starting with the bare minimum linting rules, or leaving markdown files out of the scope of these changes could help. 💭

Cross-linking relevant issue in #3841.

Hey @gtsiolis 👋 , these are just 💅🏻 code formatting changes.
The linting one I'll work on right after we sort this one out 💪 💪 💪

I already created one commit for each component, so that you can for example:

  • decide to review one component at a time by looking at its commit
  • tell me you want Prettier just for component X and component Y, and I'll easily delete all the other commits

Is this what you mean?

Re: markdown files. Personally I'd rather use Prettier for everything it supports and never spend another second of my life over code formatting, but if you prefer keeping markdown files out of the thing, I can add them to the ignored files! Just let me know 😃

@gtsiolis
Copy link
Contributor

gtsiolis commented Jan 26, 2022

Thanks, @trumbitta! I'm all in for adding prettier[1] support but would recommend a) breaking things down into smaller pieces if possible and if it makes sense in the spirit of MVC (minimum viable change) as well as b) starting with a bare minimum setup since prettier can be quite opinionated by default.

Let me pass this to the folks in @gitpod-io/engineering-webapp to decide what's the best way forward here. 🏓

@trumbitta
Copy link
Contributor Author

trumbitta commented Jan 26, 2022

Another common approach when adopting Prettier is to just have a "configuration file + format on save" setup and let it propagate changes over time.
I did it like this in the past, but it bloats PRs for months and a few files get always left behind because maybe nobody opens them for a very long time.
These days I like the YOLO approach more because once it's done you have a single point of adoption in your git history and you can start enjoying the benefits right away.

@trumbitta
Copy link
Contributor Author

trumbitta commented Feb 10, 2022

Another common approach when adopting Prettier is to just have a "configuration file + format on save" setup and let it propagate changes over time.

@gtsiolis 👋 any news? 😊

@gtsiolis
Copy link
Contributor

@trumbitta I still stand by #7757 (comment) but I'd like to defer this to the @gitpod-io/engineering-webapp team for any additional input or decide what's the best way forward here. I've also just pinged the team internally to resurface this.

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 14, 2022

@trumbitta after a short discussion about this with the team:

Thanks again for opening this! I'm going to take a bias-for-action here and close this for now as this currently seems to add little to almost no value for our users and causes more distraction for the team instead of helping us push forward our product's vision and strategy[1].

Following the comment in #7757 (comment), if you still think some of the changes in this PR could be useful, I'd recommend 🅰️ starting with a bare minimum setup and breaking changes into smaller pieces to make reviewing easier and avoid constant file conflicts as well as 🅱️ consider opening an issue to discuss beforehand such changes, their impact, etc.

Please, do not let this discourage you from contributing to other areas of the product. Thanks again for all your contributions!

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

Successfully merging this pull request may close these issues.

5 participants