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

Time type suggestions #10

Closed
RobertBColton opened this issue May 8, 2016 · 5 comments
Closed

Time type suggestions #10

RobertBColton opened this issue May 8, 2016 · 5 comments

Comments

@RobertBColton
Copy link

RobertBColton commented May 8, 2016

In the Qt Framework the DateTime, Date, and Time objects main conversion type is std::time_t. This conversion requires a rather long call:

return QVariant(QDateTime::fromTime_t(std::chrono::system_clock::to_time_t(value.as<std::time_t>())));

Is it possible that toml::Value could also provide .asstd::time_t() for time types? Is it possible that toml::Value could also distinguish between Date, Time, and DateTime? It would be useful in our Qt application which has separate QDateEdit, QTimeEdit, and QDateTime edit so as to not have to create the third control when either Date or Time is not needed.

@RobertBColton RobertBColton changed the title Constructing toml::Time has wrong type value.as<std::time_t> May 8, 2016
@RobertBColton RobertBColton changed the title value.as<std::time_t> value.as<std::time_t>() request May 8, 2016
@RobertBColton RobertBColton changed the title value.as<std::time_t>() request Time type request May 8, 2016
@mayah
Copy link
Owner

mayah commented May 8, 2016

On Linux, time_t is signed long, so I don't think it's good to have as<std::time_t>(), which is the same as as<signed long>(). It looks converting to an integer type.

It would be good to have as_time_t(), though.

mayah added a commit that referenced this issue May 8, 2016
This is a convenient method to make time_t from a Time value.
The value type must be Time. Otherwise, exception will be raised.

BUG=#10
@mayah
Copy link
Owner

mayah commented May 8, 2016

Added as_time_t().

For naming convention, asTime_t(), asTimeT() etc. can be considered. However, I want to keep the case for time_t, so I chose as_time_t(). If you have an objection, feel free to make a pull request.

@RobertBColton
Copy link
Author

I've no objections to that solution at all. Also I would like to say thank you for writing tinytoml, it is an excellently toml library and much better than the other implementations we tried. It does basically everything we need it to without much overhead or any headaches.

The only other request I have there is if I could tell the difference between a Date, a Time, and a DateTime value. The reason being that we generate our UI from the toml file. Where Qt's QVariant has DateType, TimeType, and DateTimeType there is only toml::Value::Time which does not distinguish in tinytoml. This is useful because then I know whether to create a QDateEdit, QTimeEdit, or QDateTimeEdit in Qt.

@RobertBColton RobertBColton changed the title Time type request Time type suggestions May 8, 2016
@mayah
Copy link
Owner

mayah commented May 8, 2016

According to https://github.com/toml-lang/toml#datetime, toml does not distinguish time, date or datetime.
We need to extend toml specification to distinguish them. Also, toml specification does not have a value like time (it has date and datetime, but it does not have time)?

So, currently I don't have any plan to add them by myself. The workaround would be to add type field or something in your toml file?

@RobertBColton
Copy link
Author

RobertBColton commented May 8, 2016

Thank you, I see now it would actually be impossible with std::time_t because it is since the epoch. We're going to be using schema's down the road for the components anyway, so that solution will be fine.

I decided to go ahead and mention this to the toml guys just in case they may not have considered such a use case. toml-lang/toml#412

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

No branches or pull requests

2 participants