gitoxide icon indicating copy to clipboard operation
gitoxide copied to clipboard

`gix-date` towards 1.0

Open Byron opened this issue 3 years ago • 5 comments

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.
    • [x] #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
  • [x] 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?
    • [x] 2005-04-07 format - once available use it when printing commit disambiguation information.
    • [x] A way to format all known standard formats that git is providing.
      • [ ] human format
      • [ ] local format
      • [x] 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 avatar Jul 27 '22 03:07 Byron

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: https://github.com/willstott101/gitoxide/commit/2da937707d618ed25d224d394475dd8b73e2c3e4

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 avatar Nov 27 '22 13:11 willstott101

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

willstott101 avatar Nov 27 '22 13:11 willstott101

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.

Byron avatar Nov 27 '22 18:11 Byron

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.

willstott101 avatar Nov 27 '22 19:11 willstott101

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 avatar Nov 28 '22 07:11 Byron