Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Add pre-commit automation for faster, safer development #771

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

sungchun12
Copy link
Contributor

@sungchun12 sungchun12 commented Nov 7, 2023

The goal of this PR is to remove the friction contributing to data-diff both locally and in PRs.

Problem: black formatting in the PR adds a bunch of annotations and run time that adds more friction than needed in the pull request workflow. It forces the contributor to click a lot of buttons and run black locally for those, "Oh shoot. I forgot to format!" moments. This adds a couple minutes at a time of frustration which over the course of a month, is a lot of frustration.

Solution:

  • Added pre-commit configs to automatically enforce ruff formatting in local development after a contributor runs: poetry install and pre-commit install. Ensured line length is constrained to a max line length of 120 given the previous black settings.
    image
  • Updated the formatter github action to use ruff (99.9% black compatible) which runs faster AND auto commits changes if they exist.

This wombo combo gives precious time and energy back to the contributor and declutters PRs of those annotations that should just be auto-committed.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

@sungchun12 sungchun12 self-assigned this Nov 7, 2023
@sungchun12 sungchun12 marked this pull request as ready for review November 7, 2023 17:22
@sungchun12 sungchun12 requested a review from dlawin November 7, 2023 17:22
@dlawin dlawin force-pushed the feature/pre-commit-automation branch from c2ced0e to 2108b3a Compare November 7, 2023 18:12
@sungchun12 sungchun12 force-pushed the feature/pre-commit-automation branch from 2108b3a to 839e5e6 Compare November 7, 2023 18:29
@dlawin dlawin requested a review from nolar November 7, 2023 18:35
Copy link
Contributor

@dlawin dlawin left a comment

Choose a reason for hiding this comment

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

Approved but want to give @nolar a chance to look at it as well

@dlawin dlawin merged commit c409c81 into master Nov 13, 2023
@dlawin dlawin deleted the feature/pre-commit-automation branch November 13, 2023 18:54
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.

2 participants