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

Correct humanize behaviour with datetime.date objects #726

Closed
wants to merge 3 commits into from

Conversation

skrakowi
Copy link

@skrakowi skrakowi commented Dec 4, 2019

Pull Request Checklist

Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:

  • 🧪 Added tests for changed code.
  • 🛠️ All tests pass when run locally (run tox or make test to find out!).
  • 📚 Updated documentation for changed code.
  • ⏩ Code is up-to-date with the master branch.

If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!

Description of Changes

Closes: #697

Checked "day" for daytime objects that are close to one day apart from each other
@jadchaar
Copy link
Member

jadchaar commented Dec 6, 2019

Hi @skrakowi, it seems like the CI build is still failing. Once that is fixed, we can take a look at the changes!

@skrakowi
Copy link
Author

skrakowi commented Dec 9, 2019

Hi @jadchaar, I forgot to import the factory library. I fixed it on my skrakowi/arrow library but I'm not quite sure how to resubmit a pull request or show the new changes. Do you see the factory library imported in arrow_tests.py?

@systemcatch
Copy link
Collaborator

Hi @skrakowi any changes you push to your PR branch will show up here, your factory import is present now. Just an FYI it's generally better practice to create separate feature branches on your fork rather than develop on the master branch.

The builds are failing due to a lack on coverage in arrow.py.

@skrakowi
Copy link
Author

@systemcatch Ok thanks for letting me know and for the tip! I'll keep that in mind in the future.

The coverage has been changing based on different times throughout the day and I'm not quite sure how to get 100% coverage on these tests when they depend on the current timestamp. Do you have any thoughts?

@systemcatch systemcatch changed the title Fixed Bug #697 Correct humanize() behaviour with datetime.date objects (Bug #697) Dec 11, 2019
@systemcatch
Copy link
Collaborator

@skrakowi

Here are few pointers on the circleci failures.

The new test is failing, it needs a deeper dive to understand why, from a quick glance I think the old behaviour is still occurring. I see you've added a regression test which is good, let's label it as such so others know what it's for in the future.

FAIL: test_oneday_date (tests.arrow_tests.ArrowHumanizeTests)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/crsmithdev/arrow/.tox/py38/lib/python3.8/site-packages/chai/chai.py", line 63, in wrapper

    func(self, *args, **kwargs)

  File "/home/travis/build/crsmithdev/arrow/tests/arrow_tests.py", line 1760, in test_oneday_date

    self.assertEqual(result, "a day ago")

AssertionError: '2 days ago' != 'a day ago'

- 2 days ago

? ^    -

+ a day ago

? ^

Regarding the lack of coverage, the lines that are being missed are https://github.com/crsmithdev/arrow/pull/726/files#diff-5d8eb9e97f8b648e4dae883cdce7b959R937-R942

I think if you fix the failing test coverage should sort itself out.

@jadchaar jadchaar changed the title Correct humanize() behaviour with datetime.date objects (Bug #697) Correct humanize behaviour with datetime.date objects Jan 2, 2020
@systemcatch
Copy link
Collaborator

Replaced by #748

@systemcatch systemcatch closed this Jan 2, 2020
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.

.humanize() leads to wrong result with datetime.date
3 participants