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

Add {timestamp} and {timestamp_local} placeholders #114

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Jun 14, 2021

No description provided.

@jwodder jwodder added the minor Increment the minor version when merged label Jun 14, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #114 (5761230) into master (95da3a3) will increase coverage by 27.67%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #114       +/-   ##
===========================================
+ Coverage   44.45%   72.12%   +27.67%     
===========================================
  Files           9       10        +1     
  Lines         893      897        +4     
  Branches      134      134               
===========================================
+ Hits          397      647      +250     
+ Misses        495      196      -299     
- Partials        1       54       +53     
Impacted Files Coverage Δ
src/tinuous/util.py 86.36% <ø> (+6.81%) ⬆️
src/tinuous/appveyor.py 77.38% <100.00%> (+45.23%) ⬆️
src/tinuous/base.py 87.30% <100.00%> (+25.39%) ⬆️
src/tinuous/github.py 51.00% <100.00%> (+20.50%) ⬆️
src/tinuous/travis.py 63.63% <100.00%> (+35.45%) ⬆️
src/tinuous/_version.py 100.00% <0.00%> (ø)
src/tinuous/config.py 92.63% <0.00%> (+18.94%) ⬆️
... and 3 more

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 95da3a3...5761230. Read the comment docs.

@yarikoptic
Copy link
Member

I wonder on what is the motivation/use-case this would address? I do not quite grasp on why I might need a local time stamp, and otherwise we do have all the components (year, month, etc) of the build available.

@jwodder
Copy link
Member Author

jwodder commented Jun 14, 2021

@yarikoptic
(a) Alternative date-time formats that aren't made of the broken-out pieces we provide
(b) Some users may want local time rather than UTC

@yarikoptic
Copy link
Member

@yarikoptic
(a) Alternative date-time formats that aren't made of the broken-out pieces we provide
(b) Some users may want local time rather than UTC

So, at large, {timestamp} is a convenience (underlying value is the same) over the broken down portions (but without bringing in _local adjustment), while {timestamp_local} has no similar "broken down" alternative, correct? Just makes it a little "non-orthogonal" and thus raising my question(s).

Might be overkill, but in principle it could be some {timestamp!L} custom formatter (introduced also to all the broken down components?) which we could document and use consistently without adding _local only for {timestamp} (so similar to what we used slicing for to replace abbrev flavor of commit), but didn't look in details either it would be sensibly straightforward to accomplish for all the broken down components (since would need to carry the entire datetime value to be passed into formatter somehow).

@jwodder
Copy link
Member Author

jwodder commented Jun 14, 2021

@yarikoptic

So, at large, {timestamp} is a convenience (underlying value is the same) over the broken down portions (but without bringing in _local adjustment), while {timestamp_local} has no similar "broken down" alternative, correct?

Yes.

Just makes it a little "non-orthogonal" and thus raising my question(s).

Personally, I think formatting a datetime with strftime formats is more natural than defining a separate placeholder for each field.

Might be overkill, but in principle it could be some {timestamp!L} custom formatter

I don't think implementing {timestamp_local} as {timestamp!L} is possible. For one thing, the !L would take effect after any strftime formatting.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Ok, since code-wise seems to be correct, I approve. I personally have reservations on introducing _local to timestamp while leaving all the broke down ones without zone conversion. I personally would unlikely to use _local since repository is to be shared/reused by different members of the team which might reside in different timezones, and thus adding _local would only be of a cons to others. So I leave at @jwodder discretion on adding _local handling in this PR.

@jwodder jwodder merged commit 34b7a50 into master Jun 14, 2021
@jwodder jwodder deleted the timestamp branch June 14, 2021 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants