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.

Sign up for GitHub

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

Jump to bottom

Filesize simplification #76

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Filesize simplification #76

merged 3 commits into from
Jan 10, 2023

Conversation

bbolli
Copy link
Contributor

@bbolli bbolli commented Nov 9, 2022

Changes proposed in this pull request:

  • better test coverage filesize.py
  • code simplification

bbolli added 2 commits January 8, 2023 20:06
@bbolli @hugovk
filesize: add two tests
82d680c 
Add a test for a number that exceeds the largest prefix, and a test for
a negative number. Both cases are handled correctly but were not tested.

Signed-off-by: Beat Bolli <dev@drbeat.li>
@bbolli @hugovk
filesize: simplify the logic
06145d2 
Don't handle the "gnu" format specially, instead include the separating
space directly in the unit suffix.

Also just break out of the loop when the suffix is known; the final
return statement is the same one as the one(s) formerly inside the loop.

Signed-off-by: Beat Bolli <dev@drbeat.li>
@hugovk hugovk force-pushed the filesize-simplification branch from 67637ba to 06145d2 Compare January 8, 2023 18:06
@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2023 •
edited
Loading

Codecov Report

Merging #76 (70fc85d) into main (60e3b47) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
- Coverage   99.06%   99.05%   -0.01%     
==========================================
  Files           9        9              
  Lines         745      742       -3     
==========================================
- Hits          738      735       -3     
  Misses          7        7
Flag Coverage Δ
macos-latest 97.43% <100.00%> (-0.02%) ⬇️
ubuntu-latest 97.43% <100.00%> (-0.02%) ⬇️
windows-latest 95.95% <100.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
src/humanize/filesize.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hugovk
Copy link
Member

hugovk commented Jan 8, 2023

Thanks for the PR!

Do you know why the type checking fails?

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

src/humanize/filesize.py:80: error: Returning Any from function declared to return "str"  [no-any-return]
Found 1 error in 1 file (checked 11 source files)

@hugovk hugovk added the changelog: Changed For changes in existing functionality label Jan 8, 2023
@bbolli
Copy link
Contributor Author

bbolli commented Jan 9, 2023

Do you know why the type checking fails?

Not really... My local mypy installation (v0.991) does not report any type violations in src/humanize.

@hugovk
Copy link
Member

hugovk commented Jan 10, 2023

Hmm, the same version is used by the CI:

humanize/.pre-commit-config.yaml

Lines 61 to 66 in 60e3b47

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.991
hooks:
- id: mypy
additional_dependencies: [pytest, types-freezegun, types-setuptools]
args: [--strict]

Also with the --strict option.

@hugovk
Copy link
Member

hugovk commented Jan 10, 2023

Updated to make mypy happy.

The more type-safe way is:

ret = format % (base * bytes_ / unit) + s
assert isinstance(ret, str)
return ret

But this shorter version is good enough for here. Thanks @AlexWaygood for the tips!

@hugovk
Copy link
Member

hugovk commented Jan 10, 2023

  • better test coverage filesize.py

Well, 100% to 100% is no change, and ironically by reducing the total covered lines, it's slightly decreased the total coverage percentage 🙃

  • code simplification

but this is appreciated :)

Thank you!

@hugovk hugovk merged commit 635f27a into python-humanize:main Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
changelog: Changed For changes in existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bbolli @codecov-commenter @hugovk