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

collector_guess now guesses time formats as well #424

Merged
merged 1 commit into from Jun 8, 2016

Conversation

@jimhester
Copy link
Member

@jimhester jimhester commented Jun 7, 2016

One thing to note with this is the default time format is %H:%M, so the examples in the issue are still going to be guessed as dates unless they change the time_format in the locale (as is done in the tests).

I don't know if we should change the default time format to %H:%M:%S or not.

Fixes #384

bool isTime(const std::string& x, LocaleInfo* pLocale) {
DateTimeParser parser(pLocale);
parser.setDate(x.c_str());
return parser.parse(pLocale->timeFormat_);
Copy link
Member

@hadley hadley Jun 7, 2016

Hmmmm, maybe this should default to parseTime(), since that's a more flexible default?

Copy link
Member Author

@jimhester jimhester Jun 7, 2016

I think parseTime() is too flexible actually. Using it the following tests fail

collector_guess("0001")
#> [1] "time"
collector_guess("00010203")
#> [1] "time"

Copy link
Member

@hadley hadley Jun 7, 2016

Then I'd rather make parseTime() less flexible but still correct ;)

@jimhester jimhester force-pushed the feature/issue384 branch from 05326fc to 6321fce Jun 8, 2016
@jimhester
Copy link
Member Author

@jimhester jimhester commented Jun 8, 2016

I updated parseTime() to require : in between the digits which fixed the false positives. This is a change in what it accepts though but I think it is necessary for this use case.

return false;
consumeThisChar(':');
consumeSeconds(&sec_, NULL);
bool valid = consumeInteger(2, &hour_) &&
Copy link
Member

@hadley hadley Jun 8, 2016

Do you mind sticking with the previous structure? i.e. return false when a condition isn't met - I find that easier to understand.

@hadley
Copy link
Member

@hadley hadley commented Jun 8, 2016

I think you're right that making time stricter is a good idea (but it'll need it's own bullet point in NEWS)

The flexible parser was modified to require colons between fields to
support this use case.

Fixes tidyverse#384
@jimhester jimhester force-pushed the feature/issue384 branch from 6321fce to 6bacca8 Jun 8, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Jun 8, 2016

Current coverage is 70.50%

Merging #424 into master will increase coverage by 0.24%

@@             master       #424   diff @@
==========================================
  Files            56         56          
  Lines          2811       2824    +13   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1975       1991    +16   
+ Misses          836        833     -3   
  Partials          0          0          

Powered by Codecov. Last updated by f3c181a...6bacca8

@jimhester
Copy link
Member Author

@jimhester jimhester commented Jun 8, 2016

Ok used if statement as before and added a news bullet.

@hadley
Copy link
Member

@hadley hadley commented Jun 8, 2016

LGTM

@jimhester jimhester merged commit 6cea074 into tidyverse:master Jun 8, 2016
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants