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 and apply #139

Merged
merged 5 commits into from
Feb 1, 2024
Merged

add pre-commit and apply #139

merged 5 commits into from
Feb 1, 2024

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Feb 1, 2024

Resolves #137

This change enables configuration flexibility, and the contributor can simply run all linters locally and not be flagged by CI... moreover, the installed bot can do the fixes for a user, or the user can install a git hook, so he does not need to think about lining any more...

Just after this lands or even before, it would be beneficial to install this free bot - https://github.com/marketplace/pre-commit-ci

cc: @shaypal5

@shaypal5
Copy link
Collaborator

shaypal5 commented Feb 1, 2024

Hey Borda,

Please remove all cleaning and linting changes from this PR, and separate them into two or three parts:

  1. General clean up everybody can agree on (like removing the () in a class definition I saw you suggested). I'll just approve this in a couple of minutes.

  2. Those coming with a linting tool. That PR should also include incorporating the corresponding linting tool, and an explanation on why it should replace or complement the existing linter which runs on every commit, PR, etc. I mean here things like very strict list and param list split on several lines, etc. Because generally I do not like this, and I'd like to consider before agreeing to add such an opinionated linter.

  3. Opinionated style changes suggested by you personally, not coming from a linting tool and not per se fixes (if there are any such suggestion).

This will make this PR reviewable, when focused on the topic as presented in the title.

Thank you! :)

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (28c8d0d) 99.40% compared to head (ef3380c) 100.00%.
Report is 1 commits behind head on master.

❗ Current head ef3380c differs from pull request most recent head 28f86bf. Consider uploading reports for the commit 28f86bf to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #139      +/-   ##
===========================================
+ Coverage   99.40%   100.00%   +0.59%     
===========================================
  Files           5         5              
  Lines         508       508              
  Branches       88        88              
===========================================
+ Hits          505       508       +3     
+ Misses          2         0       -2     
+ Partials        1         0       -1     
Files Coverage Δ
cachier/base_core.py 100.00% <100.00%> (ø)
cachier/core.py 100.00% <100.00%> (ø)
cachier/mongo_core.py 100.00% <100.00%> (+5.00%) ⬆️
cachier/pickle_core.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28c8d0d...28f86bf. Read the comment docs.

@Borda
Copy link
Contributor Author

Borda commented Feb 1, 2024

Please remove all cleaning and linting changes from this PR

done, resetted to the commit before apply linting

and separate them into two or three parts:

  1. General clean up everybody can agree on (like removing the () in a class definition I saw you suggested). I'll just approve this in a couple of minutes.
  2. Those coming with a linting tool. That PR should also include incorporating the corresponding linting tool, and an explanation on why it should replace or complement the existing linter which runs on every commit, PR, etc. I mean here things like very strict list and param list split on several lines, etc. Because generally I do not like this, and I'd like to consider before agreeing to add such an opinionated linter.

ahah, did a second dive to the used action, and seems the black was not used in the end, they have all been disabled by default... will fix it

  1. Opinionated style changes suggested by you personally, not coming from a linting tool and not per se fixes (if there are any such suggestion).

there were none :)

This will make this PR reviewable, when focused on the topic as presented in the title.

yes, agree

Copy link
Collaborator

@shaypal5 shaypal5 left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@shaypal5 shaypal5 merged commit 6cfa2c7 into python-cachier:master Feb 1, 2024
5 of 6 checks passed
@Borda Borda deleted the precommit branch February 1, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

switch from wearerequired/lint-action to pre-commit
2 participants