arrow-py / arrow

🏹 Better dates & times for Python
https://arrow.readthedocs.io
Apache License 2.0
8.71k stars 673 forks source link

Normalize millisecond and microsecond timestamps #796

Closed jadchaar closed 4 years ago

jadchaar commented 4 years ago

Pull Request Checklist

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

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

Add timestamp normalization to all functions that call datetime.fromtimestamp or datetime.utcfromtimestamp.

Closes: https://github.com/crsmithdev/arrow/issues/795

jadchaar commented 4 years ago

Converting to draft until I can add test cases.

codecov-commenter commented 4 years ago

Codecov Report

Merging #796 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #796   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         1735      1740    +5     
  Branches       295       295           
=========================================
+ Hits          1735      1740    +5     
Impacted Files Coverage Δ
arrow/arrow.py 100.00% <100.00%> (ø)
arrow/parser.py 100.00% <100.00%> (ø)
arrow/util.py 100.00% <100.00%> (ø)

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 f107d25...3a39f67. Read the comment docs.

jadchaar commented 4 years ago

@systemcatch @krisfremen do you guys think it is clear if I refer to this as an "expanded timestamp"? No idea what else I should refer to it as in the code/docs.

systemcatch commented 4 years ago

do you guys think it is clear if I refer to this as an "expanded timestamp"? No idea what else I should refer to it as in the code/docs.

Fine in the code, for the docs how about you push your changes and we can see how they read.

jadchaar commented 4 years ago

do you guys think it is clear if I refer to this as an "expanded timestamp"? No idea what else I should refer to it as in the code/docs.

Fine in the code, for the docs how about you push your changes and we can see how they read.

I didn't have any changes in mind, but I thought I'd ask if you had any suggestions.

systemcatch commented 4 years ago

Maybe subsecond timestamp? I don't think expanded timestamp is wrong though.

jadchaar commented 4 years ago

Maybe subsecond timestamp? I don't think expanded timestamp is wrong though.

We call it a millisecond and microsecond timestamp in the token section of the docs, but that would be clunky to write in the code. I think it is fine as is. What do you think about the code logic? We will have to fix a few merge conflicts once we merge int he format strings.

systemcatch commented 4 years ago

Let's leave it as expanded timestamp.

For the merge conflicts we should decide whether we want to haveconstants.py or not, if it's to keep I can revert that commit on my branch and avoid any conflicts.

jadchaar commented 4 years ago

Should be ready for re-review @systemcatch.