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

Add pre-commit #19

Merged
merged 2 commits into from
Jun 30, 2023
Merged

Add pre-commit #19

merged 2 commits into from
Jun 30, 2023

Conversation

ndmlny-qs
Copy link
Contributor

  • Adding pre-commit to the repository. The new pre-commit directives can be found in the configuration file .pre-commit-config.yaml.
  • Removed excess ignore items in the .gitignore for simplicity.
  • Added .taplo.toml to enforce TOML file formatting.
  • Updated the readme for the new pre-commit hooks.
  • Updated the pyproject.toml file with pre-commit configuration, as well as removing the tools handled by pre-commit from the dev install directive.

Resolves #13

- Adding `pre-commit` to the repository. The new `pre-commit` directives
  can be found in the configuration file `.pre-commit-config.yaml`.
- Removed excess ignore items in the `.gitignore` for simplicity.
- Added `.taplo.toml` to enforce TOML file formatting.
- Updated the readme for the new pre-commit hooks.
- Updated the pyproject.toml file with pre-commit configuration, as well
  as removing the tools handled by pre-commit from the dev install
  directive.

Resolves #13
@ndmlny-qs ndmlny-qs added the enhancement New feature or request label Jun 30, 2023
@ndmlny-qs ndmlny-qs self-assigned this Jun 30, 2023
@ndmlny-qs
Copy link
Contributor Author

Note that none of these hooks have been run on the codebase yet. That will be done in the issue #20

Copy link
Contributor

@aloctavodia aloctavodia left a comment

Choose a reason for hiding this comment

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

We talked about adding an option to check for "print". Other than that LGTM.

@ndmlny-qs
Copy link
Contributor Author

thanks for the reminder, I'll update for that as well

@ndmlny-qs
Copy link
Contributor Author

I left it as a flake8 error when prints are found. I think blindly removing print statements is not the greatest of dev experiences, as it removes the opportunity to convert them to a log.

We should discuss adding checks for TODO or FIXME flags that may occur as well in a future PR.

Copy link
Member

@yilinxia yilinxia left a comment

Choose a reason for hiding this comment

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

I noticed that we have two license files, is that intentional? otherwise, LGTM

@ndmlny-qs
Copy link
Contributor Author

good catch, made an issue to remove it #22

@ndmlny-qs ndmlny-qs merged commit b51bd92 into main Jun 30, 2023
@ndmlny-qs ndmlny-qs deleted the issue13/add-pre-commit branch June 30, 2023 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add pre-commit
3 participants