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

Ergonomic pre commit proposal #259

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Ergonomic pre commit proposal #259

wants to merge 5 commits into from

Conversation

emmyscode
Copy link
Contributor

@emmyscode emmyscode commented Jul 8, 2024

Context

This pull request enhances the CI process for the templates repository. Currently, we use pre-commit hooks to check for code consistency and automatically generate README.md from Jupyter notebooks.

Problems

  • When making local commits, modified files fixed by pre-commit aren't staged automatically. As a result, you need to re-stage the changes again, which isn't intuitive for contributors who are used to a single git commit motion.
  • The e2e llm template is still generating a README.md
  • Pre-commit hooks can fail on pull requests or push with opaque errors, making it difficult to debug.

Changes

Files modified

  1. .github/workflows/pre-commit.yaml
  2. .pre-commit-config.yaml
  3. ci/auto-generate-readme.sh

Functional modifications

.github/workflows/pre-commit.yaml

  • Remove redundant uses: actions/checkout@v3 because it's not needed for the pre-commit hooks to run.
  • Adding comments for better readability/style

.pre-commit-config.yaml

  • Updating external pre-commit hooks to the latest v4.6.0
  • Exclude checks for auto-generated README files; they should automatically comply with the consistency by default.
  • Configure the hook to run only during commit to avoid running it on pushes--that's redundant.

ci/auto-generate-readme.sh

  • Add set -e to make the script exit immediately if a command exits.
  • Encapsulate converting notebook logic in a function for reusability and style.
  • Ensure that all modified files are staged automatically to avoid needing to re-stage. This allows for a single git commit -s motion that's more familiar for contributors.
  • Remove the "Time to complete" check; not every template needs the hard requirement that there's a "Time to complete". Even if we did arrive at a style guide for how templates should look, we should apply these rules consistently.

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.

1 participant