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

Make seconds optional in times #696

Closed
wants to merge 3 commits into from

Conversation

eksortso
Copy link
Contributor

Allows the seconds portion of values with Offset Date-Time, Local Date-Time, and Local Time type to be omitted. Closes #671.

@eksortso eksortso changed the title I671 time w o seconds Make seconds optional in times Jan 30, 2020
@h-vetinari
Copy link

@pradyunsg @mojombo
It's a late candidate for 1.0, but very useful, since so many timestamps in real life do not actually use seconds (and adding ":00" everywhere is unnecessary dead weight for reading/typing). TOML is "a config file format for humans" after all. ;-)

@pradyunsg
Copy link
Member

Thanks for filing this PR @eksortso and for the nudge @h-vetinari!

I still think we should defer this to after 1.0; since I don't wan to pile on too many changes to 1.0 from 0.5. There's already been more than I'd have liked but they felt a lot more urgent than this does.

README.md Outdated Show resolved Hide resolved
@ChristianSi
Copy link
Contributor

Looks good now!

@h-vetinari
Copy link

@pradyunsg: I still think we should defer this to after 1.0; since I don't wan to pile on too many changes to 1.0 from 0.5. There's already been more than I'd have liked but they felt a lot more urgent than this does.

Fair enough. But OTOH, it's a very non-invasive change (optionally removing seconds does not make 0.5-documents invalid), and considering the availability of dev-time (and hence an expected long release cadence), it's something that should IMO be (re-re-)considered. ;-)

@abelbraaksma
Copy link
Contributor

Looks like this needs to be remerged with head to resolve the conflicts. From what I can tell, the changes here are sound. Anything that blocks this from going into main/master?

@RonnyPfannschmidt
Copy link

i would guess time constraints on the devs - i'd love to see this one land as well to sort out a mess i have at my hands

@eksortso
Copy link
Contributor Author

eksortso commented May 3, 2022

This PR is replaced with #894. If you contributed to this one, please look over the new one and let me know what you think over there.

@eksortso eksortso closed this May 3, 2022
@eksortso eksortso deleted the i671_time_w_o_seconds branch May 3, 2022 05:48
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.

Make seconds optional in Local time
6 participants