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

Integrate prettier to make the format consistent #5

Merged
merged 5 commits into from
Apr 10, 2022

Conversation

nvh95
Copy link
Collaborator

@nvh95 nvh95 commented Apr 9, 2022

Motivation

  • This project comprised many markdown files. However, it lacks a way to make the format consistent across the project.
  • Future Pull Requests might mess up lines that they don't edit. That affects the history line.
  • Standard formatted markdown can help third-party systems process content from this project seamlessly (e.g: memos).

How

  • Integrate prettier to backend-swe-interview-questions - a de facto tool for formating code.
  • Add a GitHub action to indicate if a pushed commit/ a Pull Request have inconsistency format.

Pending Items

  • Since we need to reformat the existing markdown files. @tamhoang1412 should be the one to do this to preserve the git history.

@nvh95 nvh95 requested review from tamhoang1412 and huytd April 9, 2022 17:51
"scripts": {
"prettier": "prettier '{,!(node_modules)/**/}*.md' --check",
"prettier:fix": "npm run prettier -- --write",
"sort": "npx sort-package-json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this sort script?

Copy link
Collaborator Author

@nvh95 nvh95 Apr 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a trick I recently have. In the future, we may install dependencies by editing package.json directly (e.g: integrate lint-stage), or update some fields in that file. To make package.json organized, just need to run npm run sort, it will sort all fields recursively.
Let's think of it as prettier, but for package.json

Copy link
Collaborator Author

@nvh95 nvh95 Apr 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, it use npx, so it does not cost any disk storage to the users (except some bytes for the script 😂😂)

@huytd
Copy link
Contributor

huytd commented Apr 9, 2022

I think the PR is pretty good so far. There is one thing I want to bring up for discussion.

Should we enforce the formatting somehow? So that every PR created will be in formatted state. I'm thinking of two approaches:

  1. Create a pre-commit hook to run Prettier before the user make any commit. The caveat is, if someone make a PR by editing directly on Github, no formatting will be done.
  2. I'm unable to check if the current Github Action will make any commit after Prettier run or not (it only check, right?). But I think it would be great if we make the action run and commit/push the formatted markdown after every PR merge. This will also solve the problem in the first approach.

What do you think? @nvh95 @tamhoang1412

@nvh95
Copy link
Collaborator Author

nvh95 commented Apr 9, 2022

@huytd

Create a pre-commit hook to run Prettier before the user make any commit. The caveat is, if someone make a PR by editing directly on Github, no formatting will be done.

I think it's a great idea. However, please remember that users clone repository locally have an option to push the changes without installing prettier 😂. So this initiative mitigates the problem, not solves it completely.

I'm unable to check if the current Github Action will make any commit after Prettier run or not (it only check, right?). But I think it would be great if we make the action run and commit/push the formatted markdown after every PR merge. This will also solve the problem in the first approach.

The current github action does not make any commits. It just check the format (Notice the flag --check, not --write). I think we can support that. But if we go with that approach, the history line of this project will be filled with reformat commits from GitHub actions, that what I personally don't like.
I think we can just check the format. If it fails, maintainer does not merge the PR but file a request change. We can also add CONTRIBUTING.md to guide them how to use prettier to format the project. We can also make a bot 🤖 to reply to PR that they fails CI checking the format and show the link to the guideline (just idea, not know how to do it now, can leave it as room for improvement in next PRs)

@nvh95
Copy link
Collaborator Author

nvh95 commented Apr 9, 2022

@huytd
For the pre-commit approach. Besides the pros you mentioned. Following are the cons:

  • I personally have bad experience integrating it. It's not a "just work" solution.
  • It takes contributors long time to finish commiting (4-5 seconds??). To me, it's a bad DX.
  • If users install the VSCode Prettier extension, pre-commit hook would not be necessary anymore (because that extension format the file on save), just wasting us a few seconds when committing

@nvh95
Copy link
Collaborator Author

nvh95 commented Apr 10, 2022

What do you think @tamhoang1412? I think the discussion is longer than the code itself. 😅

@huytd
Copy link
Contributor

huytd commented Apr 10, 2022

😂

Yeah I think just run the check and let the contributor handle the code format from now on is good enough. Let's merge this.

Btw, we can config the Github Action to commit on behalf of the repository owner, but maybe it's a bit over-engineering 😂

@nvh95
Copy link
Collaborator Author

nvh95 commented Apr 10, 2022

@huytd Yeah. Let's run it for a couple of new PRs to see how it goes and we can decide the next steps.
Thanks for your comments and feedback.

@tamhoang1412 tamhoang1412 merged commit 01151d8 into tamhoang1412:main Apr 10, 2022
@nvh95 nvh95 deleted the prettier branch April 10, 2022 07:27
@tamhoang1412
Copy link
Owner

@all-contributors please add @nvh95 for code

@allcontributors
Copy link
Contributor

@tamhoang1412

I've put up a pull request to add @nvh95! 🎉

@nvh95 nvh95 mentioned this pull request Apr 10, 2022
2 tasks
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