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 support for specifying time formats #165

Merged
merged 3 commits into from
May 15, 2023
Merged

add support for specifying time formats #165

merged 3 commits into from
May 15, 2023

Conversation

m4rch3n1ng
Copy link
Contributor

add support for specifying a format for timestamps (--time-format) in --long mode. the formats are inspired by the git implementation.

i was unsure of what flag to use, but since you used --time over --date, and therefore --time was already taken by something else, i chose the more explicit --time-format flag.

i chose to add the formats iso, iso-strict and short, the latter two strictly following their implementation in git, with iso dropping the time zone specifier entirely (since the time zone is local-only anyway), but i made it so you should be able to easily expand to more options if needed.

the old format is still the default or can be explicitly specified with --time-format default. i would personally consider using the full year instead of only the last two digits, but that would mean recreating a good number of screenshots in the readme, so i didn't

i do not know rust well enough to know how or even if to best implement unit tests, so i did not.

say when you want me to change anything. ty.

@solidiquis
Copy link
Owner

This looks great! Thank you. For future reference please refer to CONTRIBUTING.md for instructions on how to go about creating a new PR.

I won't be able to review this until the end of the week but upon a quick glance I don't think this will require any unit-testing.

@m4rch3n1ng
Copy link
Contributor Author

m4rch3n1ng commented May 2, 2023

oh i'm so sorry i actually read the CONTRIBUTING.md, but i completely missed that note at the end...

i thought about opening a seperate issue beforehand, but since i wanted to write some rust anyway and it was like a 10 minute thing i just went for it and if you were to reject it or want me to completely rewrite most of it, it would not have really mattered

thank you and take your time

@solidiquis
Copy link
Owner

No biggie! :)

@solidiquis
Copy link
Owner

solidiquis commented May 14, 2023

Hey I'm so sorry for getting back to this so late—work and personal life has been quite hectic. So by and large these changes look good and I'd like to include this as part of the next release. If it's not too much trouble, would you be able to request to merge these into the v3.0 branch instead of master? You might have to do some small refactors as I just decoupled the node module from all of the rendering logic.

The formatting of the timestamp has been moved here! Again sorry for getting back late. Let me know if you have any questions!

@m4rch3n1ng m4rch3n1ng changed the base branch from master to v3.0 May 15, 2023 14:29
@m4rch3n1ng
Copy link
Contributor Author

done i think

@solidiquis solidiquis merged commit e4de6ca into solidiquis:v3.0 May 15, 2023
@solidiquis
Copy link
Owner

Thank you! Will include it in v3.0 which I'm aiming to release before next week :]

@solidiquis solidiquis mentioned this pull request May 15, 2023
@m4rch3n1ng
Copy link
Contributor Author

ty, looking forward to it...

@m4rch3n1ng m4rch3n1ng deleted the time-formats branch June 6, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants