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

Apply and conform to better linting #197

Merged
merged 9 commits into from
Nov 28, 2023
Merged

Apply and conform to better linting #197

merged 9 commits into from
Nov 28, 2023

Conversation

jwodder
Copy link
Contributor

@jwodder jwodder commented Nov 23, 2023

Includes the commit from #196.

This revealed a typing error which appears to be a genuine bug.

duecredit/io.py Outdated Show resolved Hide resolved
@@ -31,7 +33,7 @@
from .versions import external_versions

if TYPE_CHECKING:
from collector import Citation
from .collector import Citation
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

@yarikoptic
Copy link
Member

duecredit/io.py:396: error: Argument 1 to "get_bibtex_rendering" has incompatible type "DueCreditEntry"; expected "Union[Doi, BibTeX]"  [arg-type]
Found 1 error in 1 file (checked 50 source files)

Happy Thanksgiving!!!

@jwodder
Copy link
Contributor Author

jwodder commented Nov 23, 2023

@yarikoptic Yes, that's the bug I found. As far as I can tell, mypy is right to complain: the code is trying to pass a DueCreditEntry to a function that doesn't accept DueCreditEntrys, and I don't know what the intention behind the code is.

(The reason mypy is complaining now is because, after the import fix that @a-detiste commented on above, mypy now knows what a Citation is and what its entry attributes are.)

@codecov-commenter
Copy link

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (13a0cd5) 81.50% compared to head (915fdd3) 81.29%.
Report is 6 commits behind head on master.

❗ Current head 915fdd3 differs from pull request most recent head ea49b39. Consider uploading reports for the commit ea49b39 to get more accurate results

Files Patch % Lines
duecredit/utils.py 6.66% 14 Missing ⚠️
duecredit/io.py 61.53% 5 Missing ⚠️
duecredit/__main__.py 42.85% 2 Missing and 2 partials ⚠️
duecredit/cmdline/helpers.py 50.00% 1 Missing ⚠️
duecredit/cmdline/main.py 75.00% 0 Missing and 1 partial ⚠️
duecredit/collector.py 50.00% 1 Missing ⚠️
duecredit/injections/injector.py 83.33% 1 Missing ⚠️
duecredit/log.py 75.00% 1 Missing ⚠️
duecredit/tests/test_api.py 75.00% 1 Missing ⚠️
duecredit/tests/test_dueswitch.py 87.50% 1 Missing ⚠️
... and 1 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   81.50%   81.29%   -0.21%     
==========================================
  Files          47       47              
  Lines        2546     2545       -1     
  Branches      361      359       -2     
==========================================
- Hits         2075     2069       -6     
- Misses        380      385       +5     
  Partials       91       91              
Flag Coverage Δ
unittests 81.17% <78.16%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yarikoptic
Copy link
Member

for now I think the best would be to just make it accept DueCreditEntry as type... I also RFed the get_text_rendering to accept DueCreditEntry as well instead of Citation. Will rebase to resolve conflicts now

jwodder and others added 9 commits November 27, 2023 19:05
Because nothing else but .entry of Citation were used, and so we have
it more consistent with get_bibtex_rendering which does not take Citation
Ideally I guess there should be some more elaborate class hierarchy but we have
these functions to mutate between different renderings, so for now it would be
fine to just have both  get_text_rendering and get_bibtex_rendering to just
take a generic DueCreditEntry.
@yarikoptic yarikoptic merged commit 31df99c into duecredit:master Nov 28, 2023
15 checks passed
@yarikoptic yarikoptic added the patch Increment the patch version when merged label Nov 28, 2023
@jwodder jwodder deleted the flakes branch December 5, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants