Reviewing merge request #455: QLocale::toDateTime cannot convert value correctly
the following code shows the issue:
//------------------------------------------------------------------------------------------------
QLocale locale = QLocale(QLocale::English);
//QString dateTimeStr = QString("Monday, January 5, 2009 11:48:32 AM"); // with no blank space char after "AM", this date time str cannot be parsed correctly
QString dateTimeStr = QString("Monday, January 5, 2009 11:48:32 AM ");
//QDateTime value = locale.toDateTime(dateTimeStr , locale.dateTimeFormat(QLocale::LongFormat)); // this will success
QDateTime value = locale.toDateTime(dateTimeStr , QLocale::LongFormat); // cannot return right QDateTime value
//------------------------------------------------------------------------------------------------
souce code of "toDateTime"
QDateTime QLocale::toDateTime(const QString &string, FormatType format) const
{ return toDateTime(string, dateFormat(format)); // "dateFormat" should be replaced by "dateTimeFormat" }
- Requested by:
suuuushe - Status: Merged
- Source repository: bc-qt:QTBUG-7898
- Target repository: qt:master
- Created at: February 4th, 2010 07:34
Commits that would be merged:
Comments
The patch looks good but could you add a unit test for it?
You just need add to add the two strings to tst_QLocale::toDateTime_data().
Looks fine. I agree with Benjamin – please add a test for it.
However I wonder how useful that QLocale::toDateTime() function is – the date/time format might change over time in newer versions of Qt when we upgrade to a newer version of Qt, so in practice you cannot rely on that conversion.
@Denis
You can probably rely on QLocale::toDateTime() and QLocale::dateTimeFormat() to be consistent. I guess this is useful for date editing widgets.
I change the status to “revise and resubmit”. You just need to add a test to be integrated.
sure, I have added the test case for this issue
+ void tst_QLocale::QTBUG_7898()
Do you really need a new function for this test?
tst_qlocale() already test this in tst_QLocale::formatDateTime().
I think adding two lines to tst_QLocale::formatDateTime_data() should work(?).
Don’t forget to change the status back to new when you are ready for a second review.
Benjamin Poulain |
2010-02-08 18:23:22 UTC
on tests/auto/qlocale/tst_qlocale.cpp
ok, the test code has been moved to “toDateTime”, the fail is caused by method: “QDateTime toDateTime ( const QString & string, FormatType format = LongFormat ) const”.
Also, it seems that “QLocale l("C”)“ is different from "QLocale l("en_US”)“ as if "C” is used as locale name there will be no fail. Or I might misunderstand the QLocale class, I will check it again. Thanks.
Thanks for the patch.
Merged as df09fee0363eda36d827de02a4c7e899a536ed06 to master.
Add a new comment:
Login or create an account to post a comment