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

Examine usage of repo-review tools as pre-commit hooks #4888

Closed
agriyakhetarpal opened this issue Mar 2, 2025 · 7 comments · Fixed by #4897
Closed

Examine usage of repo-review tools as pre-commit hooks #4888

agriyakhetarpal opened this issue Mar 2, 2025 · 7 comments · Fixed by #4897
Assignees
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours priority: low No existing plans to resolve

Comments

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Mar 2, 2025

To be picked as a follow-up to #3489 and #4887, this will help enforce rules around typing and check if our config is in the right place, etc. The pre-commit hook's speed must be investigated; extra thoughts are welcomed.

@agriyakhetarpal agriyakhetarpal added difficulty: easy A good issue for someone new. Can be done in a few hours priority: low No existing plans to resolve labels Mar 2, 2025
@Saransh-cpp
Copy link
Member

Just to be clear, I think this issue is independent of the issues mentioned above, and ideally, the hook should be added to the repository as soon as possible (priority is still low). We can ignore the checks that are failing right now, and then slowly un-ignore them as part of #3489.

@agriyakhetarpal
Copy link
Member Author

Sounds good to me, thanks for clarifying – I was under the impression that you want existing tests to pass first (or with the process of adding it).

@medha-14
Copy link
Contributor

medha-14 commented Mar 7, 2025

I benchmarked this, and the results are as follows:

Without the hook:
Image

With the hook:

Image

Image

@Saransh-cpp
Copy link
Member

Could you please ignore the failed checks in pyproject.toml for the time being and make this hook pass?

@agriyakhetarpal
Copy link
Member Author

The speed is reasonable, too. We should be fine with adding it, I guess.

@Saransh-cpp
Copy link
Member

Ah, I did not think about the speed at all. It should not matter if it takes a few more seconds!

@medha-14
Copy link
Contributor

medha-14 commented Mar 7, 2025

Could you please ignore the failed checks in pyproject.toml for the time being and make this hook pass?

Sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours priority: low No existing plans to resolve
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants