Rework date parsing

[evpaassen]

This addresses #169 and reworks date parsing.
Alternative implementation of #175.

[azurecla]

Hi @evpaassen, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, AZPRBOT;

[ahaarrestad]

I would suggest much more unit-tests when shortening the code like this.

[evpaassen]

But the tests cover almost all the cases. I also made sure all cases from the original method are supported. The main thing is that we don't need to parse the timezone suffix manually as this is already supported by SimpleDateFormat.

This code is actually close to the original, and doesn't refactor between methods.

[serious6]

have to admit I like the shortened version most. @evpaassen for future PR I would appreciate if you add comments and thoughts to a given PR before we break down to competitions.

[ahaarrestad]

there is one huge difference... #176 does not fix issue #169 since it does not implement fixes for the recurring appointments.

but I do agree that the reworking of the code looks better, thus my comment to merge my pr first, then do this as a step 2 :-)

[serious6]

ExchangeServiceBaseDateParsingTest is missing license-header

[evpaassen]

The license header has been added.

[serious6]

I leave it up to you how you handle this. Maybe another solution could be if @evpaassen creates a PR on your PR @ahaarrestad.

[evpaassen]

@serious6 Let's have this clear. This is not about competition, or credits or anything. Sometimes it's just easier to explain a different concept with an example. I'm just trying to be helpful here.

@ahaarrestad The tests are of course heavily inspired by your tests, as those were looking quite nice. I took one test from yours and adjusted it to verify every pattern supported by the old and new parsing code.

there is one huge difference... #176 does not fix issue #169 since it does not implement fixes for the recurring appointments.

Doesn't it? It should be fixed by f7a7c52. testStartDateWithTimeZone() and testStartDateWithTimeZoneWithColon() seem to verify it's fixed, not?

[ahaarrestad]

Your fix won't handle the actual problem that occurs if the recurring appointment contains different timezones in the string. Thus my change from the old parsing to the reworked one. Remember that this API might not be (or have been) the only input to the exchange database.....

[ahaarrestad]

@serious6 your call on how to continue #175 vs #176

[evpaassen]

OK, I reworked it a little bit again. I guess I misunderstood the original issue a little bit. It should fix #169 now, though.

It contains an additional fix for date-only parsing when the date string has no timezone. The library should then assume UTC instead of local timezone. This bug was fixed for date-time string already in #6.

[serious6]

@evpaassen, @ahaarrestad hi I aprreciate both of your contributions and encourage on this topic. Thanks for that in advance.

For solving this kind of problem we ran in yesterday maybe @ahaarrestad can provide a test which underlines the expected behavior for the recurring appointment problem, since he pointed out that this PR does not include the fix for this problem. After the integration is done or disproved maybe we can have a merge on @evpaassen PR because it contains the "easier"-implementation for the parsing methods. I dont think it is usefull to merge both of the PR. What do you think of this?

[ahaarrestad]

I'll close my pr with refs to this one. but can we try to get a merge as soon as possible?

[serious6]

@evpaassen are you finished with this? Thanks for the refactoring. I think it´s even better now.

[evpaassen]

Yes, I'm finished now.

[serious6]

Thanks for the contribution @evpaassen , @ahaarrestad
Merged with 21c7b0d

[evpaassen]

@ahaarrestad Thank you very much for your collaboration on fixing this issue. I really appreciate it.

[vboctor]

Is there a reason for throwing a different kind of exception here? IllegalArgumentException vs. ParseException? I generally prefer to use illegal argument kind of exceptions to reflect programming errors where exceptions based on runtime data would use ParseException (or FormatException in .NET). Thoughts?

[evpaassen]

The former method convertUniversalDateTimeStringToDate(...) already did so, so I kept this behaviour. Note that IllegalArgumentException is a runtime exception, where ParseException is a checked exception. I think that most important is that this behaviour is documented in the javadoc, like it is.

[vboctor]

Yep, I've just read about checked vs. runtime exceptions, I'm new to the concept of checked exceptions since it is unique to Java. However, I would have thought that parsing should be a checked exception to force the callers to handle the case where the string they are providing is not valid. This is not something that you want to bubble to the top of the stack. It is interesting however that SimpleDateFormat throws ParseException (checked) and long.ParseLong throws IllegalArgumentException (runtime). I would have thought there would be consistency there and both use checked, since I would expect parsing errors to be handled closest to where they are used.

[vboctor]

Do we want to expose this for consumers of the library? Should we make it package internal?

[evpaassen]

I placed this class in a subpackage (yes, the first subpackage in this library), so we can't make it package private if we keep it there. This shouldn't be a problem though. All major libraries or frameworks expose internal utility classes like this. Otherwise package organisation wouldn't be possible.

[vboctor]

So if we change a utility method interface, we will consider this a contract change? Hence, prefer to mark them as deprecated unless we are working on a major version? Or should we have SomeNameSpace.Internal.* to reflect that these are internal packages that shouldn't be used by client apps?

[vboctor]

@evpaassen and @ahaarrestad great to see us fixing issues as well as making the API and testability better. I was a bit late to the party, but I've added a couple of comments anyway that we can possibly address as a follow up.