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

Features/add pre commit #54

Closed
wants to merge 47 commits into from

Conversation

MaGering
Copy link
Collaborator

@MaGering MaGering commented Mar 2, 2023

Fixes #16.

Open ToDos:

  • Add mypy
  • Format digipipe/digipipe with black and isort.

Before merging into dev-branch, please make sure that

  • if data flow was adjusted: data pipeline run finishes successfully.
  • new and adjusted code is formated using black and isort.
  • the CHANGELOG.rst was updated.
  • the docs were updated.

@MaGering MaGering self-assigned this Mar 2, 2023
@MaGering
Copy link
Collaborator Author

@nesnoj this is ready for review. I've implemented the pre-commit checks with black, isort, pylint, flake8 and mypy.

Be warned, however, that the tests with pre-commit run -a do not pass for flake8, pylint and mypy. In digiplan repo we have a similar situation. We have said that we will leave the tests as they are for now and ideally take care of it in the future.

@MaGering MaGering requested a review from nesnoj March 13, 2023 14:36
@MaGering MaGering mentioned this pull request Mar 13, 2023
5 tasks
@nesnoj
Copy link
Member

nesnoj commented Mar 13, 2023

Yea, thx for this nice feature! ❤️
I haven't tested yet - one question in advance: Do you mind if we go for 80 chars line length for sake of readability?

@nesnoj
Copy link
Member

nesnoj commented Mar 14, 2023

The install instruction was buggy, install is a linux coreutil ;). Fixed in 751357a.

Another question: I added some ugly lines which were detected by pre-commit on commit. But it did not correct the files automatically which I experienced to be a very nice feature. What do you think, shall we activate this?

@MaGering
Copy link
Collaborator Author

MaGering commented Mar 14, 2023

ToDo before merge:

  • Add if possible automatic fix option to ruff.

@nesnoj nesnoj removed their request for review March 14, 2023 20:47
@nesnoj
Copy link
Member

nesnoj commented Mar 14, 2023

I removed myself as reviewer, please re-request as soon as you're done.

@MaGering MaGering mentioned this pull request Mar 20, 2023
6 tasks
@MaGering
Copy link
Collaborator Author

Closed with opened PR #64, which represents cleaned up version of this PR.

@MaGering MaGering closed this Mar 20, 2023
@nesnoj nesnoj deleted the features/add_pre-commit branch August 30, 2023 12:06
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.

2 participants