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

Support for multiple granularities in humanize #722

Merged
merged 5 commits into from Dec 9, 2019

Conversation

hwillard98
Copy link
Contributor

This implements a feature in #716 to use multiple granularities as an input to humanize.

For example,

>>> later140 = self.now.shift(seconds=8400)
>>> later140.humanize(granularity="minute")
"in 140 minutes" 
>>> later140.humanize(granularity=["hour", "minute"])
"in 2 hours and 20 minutes"

@codecov-io
Copy link

codecov-io commented Nov 26, 2019

Codecov Report

Merging #722 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #722   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines        1606   1678   +72     
  Branches      266    283   +17     
=====================================
+ Hits         1606   1678   +72
Impacted Files Coverage Δ
arrow/locales.py 100% <100%> (ø) ⬆️
arrow/arrow.py 100% <100%> (ø) ⬆️
arrow/factory.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

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

@hwillard98
Copy link
Contributor Author

@jadchaar do you have any comments or feedback for these commits?

@jadchaar
Copy link
Member

jadchaar commented Dec 2, 2019

Hey @hwillard98, apologies for the delayed response. I was quite busy this holiday weekend. At first glance, the work looks solid, but I'd like to take a closer look when I get some more free time. I talked to @systemcatch and he said he will review these PRs today as well.

@jadchaar jadchaar requested review from systemcatch and jadchaar and removed request for systemcatch December 2, 2019 16:55
Copy link
Member

@jadchaar jadchaar left a comment

Choose a reason for hiding this comment

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

Overall a brilliant PR. I need to look at your logic a bit more, but I had a bit of feedback off the bat: I would add a bit of documentation explaining and showing examples for your new feature here: https://github.com/crsmithdev/arrow/blob/master/docs/index.rst.

arrow/arrow.py Outdated Show resolved Hide resolved
arrow/arrow.py Outdated Show resolved Hide resolved
@jadchaar
Copy link
Member

jadchaar commented Dec 5, 2019

@hwillard98 thanks for the quick updates. Mind adding a section to the main documentation (you should add it to this file: https://github.com/crsmithdev/arrow/blob/master/docs/index.rst)?

It should go in the Humanize section: https://arrow.readthedocs.io/en/latest/#humanize

@hwillard98
Copy link
Contributor Author

Any other changes requested? I added the documentation in the most recent commit amend

Copy link
Collaborator

@systemcatch systemcatch left a comment

Choose a reason for hiding this comment

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

Hey @hwillard98, I only have one extra comment which is really a style preference, feel free to deal with it however you want.

Otherwise I think this PR is great and ready to go. @jadchaar any further comments?

arrow/locales.py Outdated Show resolved Hide resolved
Copy link
Member

@jadchaar jadchaar left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes we commented on previously. I got some time to look at this closer and I had a few remaining comments. Should be good to merge after these are addressed.

docs/index.rst Show resolved Hide resolved
arrow/arrow.py Outdated Show resolved Hide resolved
tests/arrow_tests.py Outdated Show resolved Hide resolved
arrow/arrow.py Outdated Show resolved Hide resolved
arrow/arrow.py Show resolved Hide resolved
Copy link
Member

@jadchaar jadchaar left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for implementing the feedback quickly 👍 .

@jadchaar jadchaar merged commit 5395fbb into arrow-py:master Dec 9, 2019
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.

None yet

4 participants