Skip to content

Conversation

@hugovk
Copy link
Member

@hugovk hugovk commented Dec 27, 2022

Changes proposed in this pull request:

@hugovk hugovk added the changelog: Changed For changes in existing functionality label Dec 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2022

Codecov Report

Merging #80 (deb767e) into main (c730de5) will decrease coverage by 0.12%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
- Coverage   99.19%   99.06%   -0.13%     
==========================================
  Files           9        9              
  Lines         741      745       +4     
==========================================
+ Hits          735      738       +3     
- Misses          6        7       +1     
Flag Coverage Δ
macos-latest 97.44% <75.00%> (-0.13%) ⬇️
ubuntu-latest 97.44% <75.00%> (-0.13%) ⬇️
windows-latest 95.97% <75.00%> (-0.12%) ⬇️

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

Impacted Files Coverage Δ
src/humanize/number.py 96.38% <0.00%> (-0.59%) ⬇️
src/humanize/i18n.py 98.36% <100.00%> (+0.02%) ⬆️
src/humanize/time.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.

Copy link
Contributor

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

Might as well fix some grammar while we're here. Otherwise, LGTM. I agree that this PR improves traceback readability.

tmp = Unit[minimum_unit.upper()]
if tmp not in (Unit.SECONDS, Unit.MILLISECONDS, Unit.MICROSECONDS):
raise ValueError(f"Minimum unit '{minimum_unit}' not supported")
msg = f"Minimum unit '{minimum_unit}' not supported"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg = f"Minimum unit '{minimum_unit}' not supported"
msg = f"Minimum unit '{minimum_unit}' not supported."

raise ValueError(
"Minimum unit is suppressed and no suitable replacement was found"
)
msg = "Minimum unit is suppressed and no suitable replacement was found"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg = "Minimum unit is suppressed and no suitable replacement was found"
msg = "Minimum unit is suppressed, and no suitable replacement was found."

@hugovk
Copy link
Member Author

hugovk commented Dec 30, 2022

I think the single sentence exception messages are fine without a full stop at the end. If there are two or more sentences, then it's needed.

A quick search of CPython source code suggests the vast majority (~95%) do not end with a full stop:

$ # Ending with a full stop
$ git grep "raise .*Error(.*\.['\"])" Lib | wc -l
      79
$ # Ending with no full stop
$ git grep "raise .*Error(.*[^\.]['\"])" Lib | wc -l
    1609

@hugovk hugovk merged commit 8f37b1c into python-humanize:main Jan 7, 2023
@hugovk hugovk deleted the flake8-errmsg branch January 7, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: Changed For changes in existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants