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

Add timespec to datetime_to_str util function. #929

Merged
merged 5 commits into from
Dec 30, 2022
Merged

Add timespec to datetime_to_str util function. #929

merged 5 commits into from
Dec 30, 2022

Conversation

imanshafiei540
Copy link
Contributor

@imanshafiei540 imanshafiei540 commented Dec 29, 2022 •
edited by gadomski
Loading

Related Issue(s): #

Description:
Currently datetime_to_str function in utils truncates all microseconds if the microsecond in datetime object is equal to zero.
I've added an optional input (timespec) to control this ability. The default behavior is like before, but If users want to retrieve the result with microseconds, they can pass timespec = "microseconds" to the datetime_to_str function, and the result with microseconds will return.

PR Checklist:

  • Code is formatted (run pre-commit run --all-files)
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

gadomski
gadomski requested changes Dec 29, 2022
View reviewed changes
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Can you add a unit test demonstrating the expected behavior? Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2022 •
edited
Loading

Codecov Report

Base: 94.34% // Head: 94.34% // Increases project coverage by 0.00% 🎉

Coverage data is based on head (c405b66) compared to base (1d56e02).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #929    /-   ##
=======================================
  Coverage   94.34%   94.34%           
=======================================
  Files          83       83           
  Lines       11983    11989     6     
  Branches     1133     1134     1     
=======================================
  Hits        11305    11311     6     
  Misses        496      496           
  Partials      182      182
Impacted Files Coverage Δ
pystac/utils.py 95.38% <100.00%> (ø)
tests/test_utils.py 95.49% <100.00%> ( 0.25%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@imanshafiei540
Copy link
Contributor Author

I've added a new unit test. Thank you so much for reviewing.

gadomski
gadomski requested changes Dec 30, 2022
View reviewed changes
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

So sorry, I should have caught this on first review ... can you also add a description of the timespec argument to the datetime_to_str docstring, e.g. at https://github.com/stac-utils/pystac/pull/929/files#diff-98860a0104c5b45415729e4d1252380cfcc767add4a1a88561d80d5cdee4828cR309. Thanks.

@gadomski gadomski added this to the 1.7 milestone Dec 30, 2022
@imanshafiei540
Add timespec to docstring.
57b4106 
Add changes in changelog file.
@imanshafiei540
Copy link
Contributor Author

No worries at all. It was my mistake. Also, add my changes in the changelog. Thanks.

@gadomski gadomski merged commit e8da685 into stac-utils:main Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gadomski gadomski gadomski approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.

Microseconds in pystac.utils.datetime_to_str result truncated if microsecond in datetime object is 0
3 participants
@imanshafiei540 @codecov-commenter @gadomski