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

gix-date towards 1.0 #471

Open
5 of 11 tasks
Byron opened this issue Jul 27, 2022 · 5 comments
Open
5 of 11 tasks

gix-date towards 1.0 #471

Byron opened this issue Jul 27, 2022 · 5 comments
Labels
C-tracking-issue An issue to track to track the progress of multiple PRs or issues

Comments

@Byron
Copy link
Member

Byron commented Jul 27, 2022

We track features we consider necessary to release version 1.0 of the git-date crate. The following listing may not be complete, and doesn't have to be in order to qualify. 1.0 can be an minimal viable product despite git supporting additional details

Features

  • switch to jiff for stability and multi-threaded now() with localtime support
    • This also gets rid of all the shenanigans related to enabling local time support in the time crate.
    • gix-date: switch from time to jiff #1474
    • Integrate log support for tracing in gix CLI as it will emit log messages.
  • parsing
    • note that format-based parsing is acceptable as the time crate is already a dependency.
    • todo: list all the source formats
  • flexible Time serialization
    • Time is actually a date-time and should be printable as date and/or time. Maybe integrate with the time crate which supports flexible formatting?
    • 2005-04-07 format - once available use it when printing commit disambiguation information.
    • A way to format all known standard formats that git is providing.
      • human format
      • local format
      • raw and unix format
    • a super-simple gix log that allows to set the time and prints a git-log line by line. Validate that the printed dates are correct and there is no mismatch between UTC/local times.
@Byron Byron added the C-tracking-issue An issue to track to track the progress of multiple PRs or issues label Jul 27, 2022
@Byron Byron mentioned this issue Aug 3, 2022
60 tasks
@Byron Byron mentioned this issue Aug 19, 2022
2 tasks
@willstott101
Copy link
Contributor

Having a look at this as it seems like an easy introduction. I assume the goal with human time is not to replicate git exactly, but to take the concept and run with it a bit?

WIP: willstott101@2da9377

Have some comments there re when git (2.37.2) chooses to show timezones seeming a bit weird to me.

Another question that might help me finish this off: Should I prevent the time crate from leaking into public API? I assume we do want to avoid leaking it, but checking as it might influence a few choices.

@willstott101
Copy link
Contributor

willstott101 commented Nov 27, 2022

Final question re(garding) local, would we like that to be an additional boolean argument to Time::format, or more like time.local().format()?

@Byron
Copy link
Member Author

Byron commented Nov 27, 2022

Having a look at this as it seems like an easy introduction.

I am glad you are giving it a shot, thank you!

I assume the goal with human time is not to replicate git exactly, but to take the concept and run with it a bit?

As baseline, human parsing should support what git does. If 'running with it' means to make improvements, like to fix shortcomings, or add more parseable input that will improve the user experience, I'd be open to that, too.

Have some comments there re when git (2.37.2) chooses to show timezones seeming a bit weird to me.

I think it's important to do the same thing, unless it's clearly a bug. Here that doesn't seem to be the case though. However, there could be configuration options to not display the timezone, for example, to allow further customization. It's perfectly fine to do that as long as it's possible to get git behaviours.

Another question that might help me finish this off: Should I prevent the time crate from leaking into public API?

It's currently exposed and it can remain exposed. The time crate I consider 'safe' to rely on and would rather add fixes upstream than to try to abstract everything. In other words, time is there to stay and is a trusted building block.

Final question re local, would we like that to be an additional boolean argument to Time::format, or more like time.local().format()?

(I have to assume that re means regarding) That's a great question. We could use a builder somewhere to make it more configurable and extendable. For instance, format(…) could return a struct that implements Display, but can also be configured in other ways. Alternatively, the Format enum can take additional data to configure the format, and I think that's just as powerful. Furthermore, using the current time is a side-effect, and the current time to assume where it matters could be passed via Format instead.

Anyway, I think it will be easier to discuss this over a PR, moving forward.

@willstott101
Copy link
Contributor

However, there could be configuration options to not display the timezone, for example, to allow further customization. It's perfectly fine to do that as long as it's possible to get git behaviours.

Ok, I'll try and do what git does as closely as I can. I'd be inclined to say that adding more format types, rather than additional flags or options might provide an easier to understand API surface. But regardless I think I'll keep the scope of the incoming PR to replicating git.

Alternatively, the Format enum can take additional data to configure the format, and I think that's just as powerful.

The local is only meaningful on some of the formats, so including in the enum might be the cleanest option.

Anyway, I think it will be easier to discuss this over a PR, moving forward.

Absolutely.

@Byron
Copy link
Member Author

Byron commented Nov 28, 2022

I'd be inclined to say that adding more format types, rather than additional flags or options might provide an easier to understand API surface.

It's definitely something you can experiment with to see what feels right.

Looking forward to seeing you over in the PR section :).

@Byron Byron changed the title git-date towards 1.0 gix-date towards 1.0 Sep 4, 2023
BurntSushi added a commit to BurntSushi/gitoxide that referenced this issue Jul 28, 2024
This should make the swap from `time` to `jiff` easier.

This comment[1] indicates that it's okay for `time` to be a public
dependency, but since this patch series is about swapping `time` for
`jiff`, it seemed appropriate to take this step first. And in
particular, it was *almost* already the case that `time` was a private
dependency of `gix-date`. The only thing we really had to button up was
the exposure of `time`'s custom formatting description language.

Jiff doesn't support `time`'s custom formatting machinery and instead
uses a strftime/strptime like API. We could expose that instead, but
since nothing (other than a test) was actually utilizing `time`'s custom
formatting machinery external to `gix-date`, I figured we might as well
completely encapsulate it.

[1]: GitoxideLabs#471 (comment)
LuaKT pushed a commit to LMonitor/gitoxide that referenced this issue Aug 20, 2024
This should make the swap from `time` to `jiff` easier.

This comment[1] indicates that it's okay for `time` to be a public
dependency, but since this patch series is about swapping `time` for
`jiff`, it seemed appropriate to take this step first. And in
particular, it was *almost* already the case that `time` was a private
dependency of `gix-date`. The only thing we really had to button up was
the exposure of `time`'s custom formatting description language.

Jiff doesn't support `time`'s custom formatting machinery and instead
uses a strftime/strptime like API. We could expose that instead, but
since nothing (other than a test) was actually utilizing `time`'s custom
formatting machinery external to `gix-date`, I figured we might as well
completely encapsulate it.

[1]: GitoxideLabs#471 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue An issue to track to track the progress of multiple PRs or issues
Projects
None yet
Development

No branches or pull requests

2 participants