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

Add pre-commit hooks for formatting and linting #10737

Merged
merged 42 commits into from
Jun 10, 2024

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Apr 3, 2024

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Hand-formatting code is a drain on team productivity:

  • slows development
  • easily forgotten
  • bloats diffs
  • causes merge conflicts
  • prolongs code reviews or encourages bikeshedding
  • perhaps most importantly: could discourage people from contributing

This PR adds prettier to format the frontend. Run it with yarn prettier:fix.

It also adds a GitHub action that fails PRs without formatting. A hint is given:

[warn] Code style issues found in the above file. Run Prettier to fix.

Of course, that's a last resort, as pull request time is too late to get the real benefit. We can socialize the knowledge to enable either "format on save" in VS Code or install a pre-commit hook (update: included here).

Demonstration

Sample changes that would be made in other pending work:
1e79453
7629b6d

Issues Solved

Closes #10575

Checklist

  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Ticket Background

  • Sponsored by: Getty Conservation Institute
  • Found by: @
  • Tested by: @
  • Designed by: @

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

Overall I like this feature. There are some merge conflicts that need to be taken care of.

I'm wondering if instead of something like yarn format , this could be added as a pre-commit hook to remove the friction of using this

@jacobtylerwalls
Copy link
Member Author

I'm wondering if instead of something like yarn format , this could be added as a pre-commit hook to remove the friction of using this

Sure, I was planning to add something like that as a follow-up, but I'm happy to add a little PoC here.

@jacobtylerwalls jacobtylerwalls force-pushed the jtw/add-prettier-config branch from 4aa63c6 to 6562297 Compare April 11, 2024 20:35
@jacobtylerwalls jacobtylerwalls force-pushed the jtw/add-prettier-config branch from 6562297 to 94fedb5 Compare April 11, 2024 21:22
@jacobtylerwalls
Copy link
Member Author

Added a pre-commit hook so that folks can have a frictionless formatting experience.

To use, assuming you already have the new js dependencies like prettier itself:

pip install -r arches/install/requirements_dev.txt
pre-commit install

Then write some vue/ts by hand, attempt to commit, see the auto-fix happen as the commit aborts. You can always git commit --no-verify.

I can walk interested folks through this on the forum and update the contributor docs if this lands. It is truly optional, if you like waiting for Github Actions to fail and asking you to make more commits ;)

@jacobtylerwalls jacobtylerwalls changed the title Add command to format vue/ts files #10575 Add pre-commit hook to format vue/ts files #10575 Apr 12, 2024
@jacobtylerwalls jacobtylerwalls force-pushed the jtw/add-prettier-config branch from 0fbdc00 to 24b7d77 Compare April 26, 2024 20:49
@jacobtylerwalls jacobtylerwalls changed the title Add pre-commit hook to format vue/ts files #10575 Add pre-commit hooks for formatting Apr 26, 2024
@jacobtylerwalls
Copy link
Member Author

@chrabyrd added black, but decided to wait on eslint/ts until #10758.

@jacobtylerwalls jacobtylerwalls force-pushed the jtw/add-prettier-config branch from 0a49af3 to 2b7f294 Compare June 3, 2024 15:08
@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Jun 3, 2024

Merge checklist:

@jacobtylerwalls jacobtylerwalls force-pushed the jtw/add-prettier-config branch from 2b7f294 to 479e7cc Compare June 3, 2024 15:26
@jacobtylerwalls jacobtylerwalls requested a review from a team as a code owner June 3, 2024 15:26
@jacobtylerwalls
Copy link
Member Author

👋 @chrabyrd ready for another look, let me know if you have any questions. Will need to repoint package.json dev dependencies back to 7.6.x before merging.

@jacobtylerwalls jacobtylerwalls requested review from chrabyrd and removed request for a team June 3, 2024 15:37
@chrabyrd
Copy link
Contributor

chrabyrd commented Jun 5, 2024

combed the code and it looks good. Going to do one final runthrough to watch it work && it should be g2g 👍

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

Gave it another lookover and runthrough and it looks and works well. There's just a few minor things to address:

  1. I notice .pre-commit-config.yaml gets added to the interpolation logic in updateproject.py, but the file is never shutil-ed over.
  2. Can updateproject.py and arches-admin/arches-project get updated to run pre-commit install? That way the user doesn't need to do that extra step
  3. Can the 7.6.0 doc be updated to include instructions for this feature? Even if all the work is abstracted away in updateproject at least the line that says they need to reinstall dev requirements.
  4. Is this involved enough to include documentation around in arches-docs?

But yeah overall these are relatively minor and this branch is nearly wrapped up. This work should undo a lot of the friction for styleguide adhesion 👍

@jacobtylerwalls
Copy link
Member Author

I notice .pre-commit-config.yaml gets added to the interpolation logic in updateproject.py, but the file is never shutil-ed over.

Great catch, thanks.

Can updateproject.py and arches-admin/arches-project get updated to run pre-commit install? That way the user doesn't need to do that extra step

I'm open to it, but it's kind of breaking new ground, since I don't think updateproject or arches-admin/etc. manage python dependencies, e.g. installing the dev dependencies. I'd like to get some more input here, and would suggest to not block this work on it.

Another wrinkle is that folks might choose to pre-commit install --allow-missing-config if they're regularly switching between 7.6 and 7.5 or project/7.6.x and project/7.5.x, but I don't necessarily want to make that decision for everyone. Was going to explain that in the forum post.

Can the 7.6.0 doc be updated to include instructions for this feature? Even if all the work is abstracted away in updateproject at least the line that says they need to reinstall dev requirements.

agree 👍

Is this involved enough to include documentation around in arches-docs?

Yep, I'll raise a PR to the creating-a-development-environment section, especially if we end up not auto-installing dev dependencies.

@jacobtylerwalls jacobtylerwalls requested a review from chrabyrd June 10, 2024 14:20
@chrabyrd
Copy link
Contributor

Can updateproject.py and arches-admin/arches-project get updated to run pre-commit install? That way the user doesn't need to do that extra step

I'm open to it, but it's kind of breaking new ground, since I don't think updateproject or arches-admin/etc. manage python dependencies, e.g. installing the dev dependencies. I'd like to get some more input here, and would suggest to not block this work on it.

Another wrinkle is that folks might choose to pre-commit install --allow-missing-config if they're regularly switching between 7.6 and 7.5 or project/7.6.x and project/7.5.x, but I don't necessarily want to make that decision for everyone. Was going to explain that in the forum post.

Yeah that's fair and a good point. As long as there's solid documentation around this then I'm good to move forward without that bit of logic 👍

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

lgtm! Great feature to have and thanks for implementing it 💯

@jacobtylerwalls jacobtylerwalls merged commit c87c7de into dev/7.6.x Jun 10, 2024
5 checks passed
@jacobtylerwalls jacobtylerwalls deleted the jtw/add-prettier-config branch June 10, 2024 16:41
@jacobtylerwalls
Copy link
Member Author

Of course, thanks for the review! Next stop: forum

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.

Add prettier configuration to format vue/ts files
2 participants