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

Change request: Emit error instead of warning for: "All formats failed to parse. No formats found." #923

Open
aryoda opened this issue Sep 25, 2020 · 2 comments

Comments

@aryoda
Copy link

aryoda commented Sep 25, 2020

Context

Currently lubridates ymd, dmy etc. parsing functions work by guessing the best matching date format using a subset of the input data (see .get_train_set for the algorithm which is currently based on primes):

Current behaviour

If the subset contains only invalid or no dates this warning is thrown even if the date format could be derived from other correct dates that are not contained in the subset:

# https://github.com/tidyverse/lubridate/blob/f9f36a09f5d02b3fedf478bd93e7f547cac65752/R/parse.r#L630
warning("All formats failed to parse. No formats found.", call. = FALSE)

Impact

Since warnings do not stop the program execution in R

  1. wrong results can be overlooked easily (the results of subsequent logic may be wrong without seeing an error)
  2. errors in other parts of the user's R code due to non-parsed dates are harder to track-back to the real reason (failed parsing of dates earlier).
  3. Also note: Unit tests may not discover parsing errors with given predefined test data but in production just a different number of elements to be parsed may cause problems (= no formats found) due to the "sampling" (subsetting) logic to recognize the date format. This has the same impact as 1, but with a "good unit test feeling", that everything is working.

Suggestions

  1. Throw an error instead of a warning and
  2. add an option to configure if you want to get an error or a warning (with an error as default value)

I also suggest to improve the warning message to indicate a solution for the problem.
This may possibly reduce the number of questions for this on SO and here ;-)

warning("All formats failed to parse. No formats found. Use parse_date_time with exact=TRUE instead.", call. = FALSE)

See also

#897

@vspinu
Copy link
Member

vspinu commented Sep 28, 2020

Good idea. Thanks!

@dlebauer
Copy link

dlebauer commented Jul 11, 2024

I like this idea.

I also want to highlight the suggested change to the message. It always takes me a few moments to say 'aha I recognize that warning as a date parsing error from previous experience'

Regarding the message, having an error would at least make it possible to use traceback() to find the cause.

But I think a more specific warning (or error) could help debugging if it printed the string/number that wasn't recognized and the function name like

warning("lubridate function ymd couldn't convert '11 22 3' to a date")

I'll also point out that the current behavior of throwing a warning is inconsistent with documentation that indicates failure to parse should be an error

##' `All formats failed to parse` error. In such cases please see

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

3 participants