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

Linting and code formatting #2

Merged
merged 11 commits into from
Jun 26, 2023
Merged

Linting and code formatting #2

merged 11 commits into from
Jun 26, 2023

Conversation

katoss
Copy link
Owner

@katoss katoss commented Jun 14, 2023

Hi everyone,

in this PR/on this branch I'm trying to collect commits related to general code formatting and linting.

These are the issues I've addressed or plan to address (I added names according to who gave me feedback on this, but of course feel free to review any of them!):

  1. Linting
  • @Robaina, @Batalex, @khynder
  • 4e1a75b, 91482b1: Added black to poetry and formatted the analysis file with it
  • Open Question: I am wondering now where to best integrate black, I read that it is possible as pre-commit hook and in the CI (as suggested by @Robaina ). Maybe even both? Then there would be a local check before committing, and also a later check in case some contributor does not use the pre-commit hook. What do you think?
  1. Type hints
  1. Specific formatting issues:

Best,
@katoss

@katoss katoss requested a review from Robaina June 14, 2023 17:36
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 95.83% and project coverage change: +0.21 🎉

Comparison is base (2ed2dba) 79.38% compared to head (0a2b4b1) 79.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #2      +/-   ##
==========================================
+ Coverage   79.38%   79.59%   +0.21%     
==========================================
  Files           2        2              
  Lines          97       98       +1     
==========================================
+ Hits           77       78       +1     
  Misses         20       20              
Impacted Files Coverage Δ
src/cardsort/analysis.py 78.94% <95.65%> (+0.22%) ⬆️
src/cardsort/__init__.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Batalex
Copy link
Collaborator

Batalex commented Jun 15, 2023

You can take a look at https://pre-commit.ci/ so that you can integrate black in your CI

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@Robaina Robaina left a comment

Choose a reason for hiding this comment

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

Nice use of typing! much clearer now

src/cardsort/analysis.py Outdated Show resolved Hide resolved
@katoss
Copy link
Owner Author

katoss commented Jun 20, 2023

You can take a look at https://pre-commit.ci/ so that you can integrate black in your CI

@Batalex I have not entirely understood this service yet. I add black to the pre-commit file (which I haven't created it), and then this service will automatically perform the precommit as part of the CI?

@Batalex
Copy link
Collaborator

Batalex commented Jun 20, 2023

You can take a look at https://pre-commit.ci/ so that you can integrate black in your CI

@Batalex I have not entirely understood this service yet. I add black to the pre-commit file (which I haven't created it), and then this service will automatically perform the precommit as part of the CI?

Yes, that is the idea. I have yet to use this service myself, so this is not by any means required for our review process.

@katoss
Copy link
Owner Author

katoss commented Jun 21, 2023

Yes, that is the idea. I have yet to use this service myself, so this is not by any means required for our review process.

I tried it and it seems to work :) I will convert this draft to a PR, I think we are good to go with this one, too

@katoss katoss marked this pull request as ready for review June 21, 2023 14:30
Copy link
Collaborator

@Batalex Batalex 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 a few typos in the code (treshold instead of threshold). It's up to you if you want to fix it in this PR.
Otherwise, LGTM ✔︎

5., 5., 5., 5., 4., 5., 5., 5., 5., 5., 5., 5.,
2., 1., 4., 3., 3., 5., 3., 2., 1., 1., 5., 3.,
3., 3., 5., 2., 2., 5., 0., 4., 4.]
expected = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you'd like, you could use comments to disable black for these lines:

# fmt: off
expected = [...]

# fmt: on

Copy link
Owner Author

Choose a reason for hiding this comment

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

oh yes, great, I already wondered if this is really the intended behavior in this case, listing one value in each line. I didn't know that black could be disabled for specific lines, will do

@katoss katoss merged commit 57697d5 into main Jun 26, 2023
3 checks passed
@katoss katoss deleted the formatting-linter branch June 26, 2023 15:23
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.

3 participants