kotlinx-datetime icon indicating copy to clipboard operation
kotlinx-datetime copied to clipboard

DateTimeFormatException is internal

Open jurmous opened this issue 3 years ago • 11 comments

Currently DateTimeFormatException is internal so it cannot be used in projects to catch this specific exception.

Proposed Solution: Make it public like IllegalTimeZoneException and DateTimeArithmeticException

jurmous avatar Oct 28 '21 13:10 jurmous

Hi! Could you please give some use cases for this? Why can't you instead catch the more generic IllegalArgumentException?

dkhalanskyjb avatar Oct 28 '21 13:10 dkhalanskyjb

I can handle the missing export indeed with the generic IllegalArgumentException. But it could be that I want to have a higher level try catch in which I want to differentiate failures of specific calls. Now a try catch is needed directly around the method call to differentiate illegal argument exceptions.

But my main issue is that it was not consistent. IllegalTimeZoneException is also an IllegalArgumentException but that one is public.

jurmous avatar Nov 01 '21 16:11 jurmous

But it could be that I want to have a higher level try catch [...]

Sure, it could be, but did it actually happen? If so, could you please describe the high-level structure of the code that led to this?

But my main issue is that it was not consistent.

We want to keep the API surface as slim as viable. We managed to think of some places in code where IllegalTimeZoneException could have its uses, but DateTimeFormatException did not look like something that could meaningfully be recovered from at a higher level. If there is a use case that we've missed, let's discuss it, we're by no means against making this exception public if this is actually needed!

dkhalanskyjb avatar Nov 02 '21 09:11 dkhalanskyjb

I was also confused by this.

I expected a parse exception of some kind like java.time.format.DateTimeParseException - and even tried to catch the DateTimeFormatException found in the stack trace only to find that is internal (even though it is obviously thrown as part of the API).

The current solution is non-obvious - and making the DateTimeFormatException public would be both clearer and increase specificity, while also maintaining backwards compatibility with existing code.

akafred avatar Apr 15 '22 09:04 akafred

The current solution is non-obvious

The current solution to what exactly? What are you trying to do that is difficult to do currently that would become easier if the exception was public? As mentioned above, we're not against making it public if someone made an argument for it.

dkhalanskyjb avatar Apr 15 '22 09:04 dkhalanskyjb

It's thrown for the caller to handle, so it's part of the public contract for parse(). It being marked as internal inherently clashes with that. And there's no nullable parse() at the moment, so in order to parse at all I'm forced to read the library's source to figure out which base exception you expect me to catch instead. I would have less issue with it if the developer experience surrounding exceptions was better, but at the moment it's pretty bad.

RFonzi avatar Apr 24 '22 19:04 RFonzi

It's thrown for the caller to handle, so it's part of the public contract for parse()

The public contact of parse() is that it throws an IllegalArgumentException, it's stated in its documentation. The caller can handle it by catching this type of exception.

Regarding nullable variants of parse() functions, we may provide them at some point, like we have already done for Duration type, for example.

ilya-g avatar May 13 '22 20:05 ilya-g

The public contact of parse() is that it throws an IllegalArgumentException, it's stated in its documentation.

I think that's what's throwing me off. That documentation isn't present in the actual implementation on jvm. I had to specifically search for the expect definition via Find. It wasn't clear this was documented.

🙏 I would love a nullable version though, so I don't have to do any of that ^

RFonzi avatar May 18 '22 18:05 RFonzi

An elaboration of the arguments mentioned:

  • The current behavior is confusing (aka non-obvious) - suggested by the fact that this is reported and commented on by several users; I believe the main issue is that throwing an internal exception is non-idiomatic, even as a subclass of a documented exception
  • An exception thrown is already part of the API so claiming that it slims the API by keeping it internal is mute (if it was thrown as the cause of an IllegalArgumentException it would perhaps be different)
  • Being able to catch it would make it possible to increase the specificity of the exception, making it clear that the parsing failed, and it wasn't e.g an issue with a values being out of range (month 13 or something)

akafred avatar Oct 07 '22 17:10 akafred

With the latest release, parseOrNull function is now available. For example, LocalDate.Formats.ISO.parseOrNull("2020-01-XX"). If you are expecting an exception, it's more idiomatic to call parseOrNull and check the result. DateTimeFormatException is now for cases when the input is not expected to be non-conformant. Does this solve the issue?

dkhalanskyjb avatar Mar 07 '24 15:03 dkhalanskyjb