Skip to content

Conversation

@amrit110
Copy link
Member

@amrit110 amrit110 commented May 3, 2024

PR Type

[Feature | Fix | Documentation | Other ]
CI

Short Description

  • Adds default config for using pre-commit ci (https://pre-commit.ci/).
  • If this PR is approved, I will give permissions to the CI to submit PRs for fixing pre-commit errors and also updating the hooks

Clickup Ticket(s): Link(s) if applicable.

Add a short description of what is in this PR.

Tests Added

Describe the tests that have been added to ensure the codes correctness, if applicable.

@amrit110 amrit110 requested review from emersodb and lotif May 3, 2024 20:18
@amrit110 amrit110 self-assigned this May 3, 2024
@amrit110
Copy link
Member Author

amrit110 commented May 6, 2024

Currently failing due to pip-audut check. Posted an update which would fix that #133

Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

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

I think this is good with me, but worth @lotif also double checking to see if there are any implications I might be missing.

@emersodb
Copy link
Collaborator

emersodb commented May 6, 2024

@amrit110: pip-audit whack-a-mole going on here 😂. Feel free to just include the fix/ignores in this PR

autoupdate_branch: ''
autoupdate_commit_msg: '[pre-commit.ci] pre-commit autoupdate'
autoupdate_schedule: weekly
skip: [mypy]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add tests to the skip list too? I think the goal of this is to perform the auto-corrections from black, isort and such, right? Please correct me if I'm wrong.

We should definitely skip the smoke tests, though, or this will take a long time to finish (and also because of that flakiness that is still happening sometimes, albeit a bit rarely).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see, there are no tests in the pre-commit for this repo. Approving :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think we should have tests in the pre-commit (would be a different PR)? I didn't include them, because I didn't necessarily want to preclude people from intermediate (but test breaking) commits. However, I could be convinced that was not a good idea 😂.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a good idea if the tests are fast to run. The Florist unit tests are super fast (maybe because there are not that many yet) so I included them, but left the integration tests out.

@amrit110 amrit110 merged commit 7e56a15 into main May 7, 2024
@amrit110 amrit110 deleted the enable_pre_commit_ci branch May 7, 2024 14:03
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.

4 participants