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

🎨Pre commit #300

Closed
wants to merge 5 commits into from
Closed

🎨Pre commit #300

wants to merge 5 commits into from

Conversation

mayankpatibandla
Copy link
Contributor

@mayankpatibandla mayankpatibandla commented Oct 19, 2023

Summary:

Adds pre-commit to run some hooks that must pass before a commit is allowed. Some basic errors regarding whitespace and unused variables have been fixed.

Motivation:

The goal is to enforce a more consistent coding style and fix linting issues. Currently most errors have been disabled, but we will need to enable some of them later to comply to a chosen standard.

Test Plan:

  • To set up pre-commit, run pip install pre-commit if you don't already have it, and then run pre-commit install in the project directory. Pre-commit hooks should now run automatically every time a new commit is made.
  • Make sure that pre-commit run --all-files passes for all files in the project.

@mayankpatibandla mayankpatibandla marked this pull request as ready for review October 25, 2023 19:49
@ayushuk
Copy link
Member

ayushuk commented Jan 22, 2024

From the diff I can't see the difference between this and #301. What was the purpose of this PR again?

@mayankpatibandla
Copy link
Contributor Author

#300 got merged into #301 so that #301 could be tested. You originally said that you wanted me to put them in two separate PRs. Do you still want that or can we get rid of #300?

@ayushuk
Copy link
Member

ayushuk commented Jan 22, 2024

Oh right. Yeah I think we can close #300 since I think I was trying to be safe before doing a release. Since we are merging this after a release we should be able to merge everything together since #301 has everything. Feel free to close this PR and lets merge only #301.

@mayankpatibandla
Copy link
Contributor Author

Closed because this PR got merged into #301

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.

None yet

2 participants