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

feat(python): Adding support for pre-commit linter and formatter based on ruff #1892

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kirgrim
Copy link

@kirgrim kirgrim commented Feb 7, 2025

Checklist (if applicable):

@kirgrim kirgrim requested a review from Irillit February 7, 2025 15:15
@kirgrim kirgrim self-assigned this Feb 7, 2025
@github-actions github-actions bot added feature New feature or request python Python config root labels Feb 7, 2025
@kirgrim kirgrim force-pushed the hrymailo/feat/pre-commit-linting-and-formatting branch 2 times, most recently from 710addf to ef23ac6 Compare February 7, 2025 15:25
@kirgrim kirgrim requested a review from yesudeep February 7, 2025 15:27
@kirgrim kirgrim force-pushed the hrymailo/feat/pre-commit-linting-and-formatting branch from ef23ac6 to 5e44bc4 Compare February 7, 2025 16:11
@kirgrim
Copy link
Author

kirgrim commented Feb 7, 2025

@yesudeep @Irillit I will temporarily disable linter checks in gh actions, as it will require much refactoring which is related to #1820 and more

Copy link
Contributor

@yesudeep yesudeep left a comment

Choose a reason for hiding this comment

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

We already have pre-commit checks in captainhook.json. Can you please use that? I don't want us to use 2 different sets of commands because some of us are working on both go and python. These pre-commit checks already exist?

Please edit this file: https://github.com/firebase/genkit/blob/main/py/captainhook.json for the changes

@yesudeep
Copy link
Contributor

yesudeep commented Feb 7, 2025

@yesudeep @Irillit I will temporarily disable linter checks in gh actions, as it will require much refactoring which is related to #1820 and more

Sounds good

@@ -208,6 +207,7 @@ function genkit::install_docs_cli_tools() {
# Install pre-commit hooks.
function genkit::install_pre_commit_hooks() {
captainhook install -f -c "${TOP_DIR}/py/captainhook.json"
pre-commit install
Copy link
Contributor

@yesudeep yesudeep Feb 7, 2025

Choose a reason for hiding this comment

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

Please edit https://github.com/firebase/genkit/blob/main/py/captainhook.json and https://github.com/firebase/genkit/blob/main/py/bin/fmt#L36 as required. They are already managing formatting, pre-commits, pre-pushes, etc.

Copy link
Author

Choose a reason for hiding this comment

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

Please edit https://github.com/firebase/genkit/blob/main/py/captainhook.json and https://github.com/firebase/genkit/blob/main/py/bin/fmt#L36 as required. They are already managing formatting, pre-commits, pre-pushes, etc.

I dont really like using captainhook and fmt for these purposes for several reasons:

  1. it requires 'golang' depedency which couples our pythonic solution with golang dependencies which i believe is non-optimal
  2. pre-commit is a native and commonly accepted solution for linting and formatting which is familiar for most of python devs
  3. creating a separate script for formatting and separate for linter definitions is redundant when we could have everything in one place nicely structured, supported and up-to-date

@Irillit @timere-nemo what do you think of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please edit https://github.com/firebase/genkit/blob/main/py/captainhook.json and https://github.com/firebase/genkit/blob/main/py/bin/fmt#L36 as required. They are already managing formatting, pre-commits, pre-pushes, etc.

I dont really like using captainhook and fmt for these purposes for several reasons:

1. it requires 'golang' depedency which couples our pythonic solution with golang dependencies which i believe is non-optimal

2. pre-commit is a native and commonly accepted solution for linting and formatting which is familiar for most of python devs

3. creating a separate script for formatting and separate for linter definitions is redundant when we could have everything in one place nicely structured, supported and up-to-date

@Irillit @timere-nemo what do you think of it?

I would like to have less dependencies and proceed with pre-commit.

Copy link
Contributor

@yesudeep yesudeep Feb 10, 2025

Choose a reason for hiding this comment

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

I understand Python devs prefer pre-commit just like JS devs prefer husky and Go devs may have their own.
We have cross-runtime unit tests and code generation going on, I'd prefer not to have to maintain multiple commit managers since these bits need to run on the same machine while working on any of these runtimes and some of us work on multiple runtimes (I am working on both Go and Python).

For example, if I'm editing https://github.com/firebase/genkit/blob/main/tests/specs/reflection_api.yaml and fix only the JS affected while leaving the Python broken, my code changes would be incomplete.

Please keep your hooks in captainhook.json for now. We should focus on some more important tasks at the moment.

@github-actions github-actions bot added docs Improvements or additions to documentation js go tooling dotprompt sample labels Feb 10, 2025
@kirgrim kirgrim force-pushed the hrymailo/feat/pre-commit-linting-and-formatting branch 2 times, most recently from 7774bc8 to 2ffe49b Compare February 10, 2025 12:23
@kirgrim
Copy link
Author

kirgrim commented Feb 10, 2025

@yesudeep @Irillit I will temporarily disable linter checks in gh actions, as it will require much refactoring which is related to #1820 and more

Sounds good

I did formatting/linting updates in this PR in-place so uncommented that back

@kirgrim kirgrim force-pushed the hrymailo/feat/pre-commit-linting-and-formatting branch from 2ffe49b to 77ad22b Compare February 10, 2025 13:17
Copy link
Contributor

@Irillit Irillit left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config docs Improvements or additions to documentation dotprompt feature New feature or request go js python Python root sample tooling
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants