Fix broken tests in Datetime Parser

[hishamco]

Related to #10601

[deanmarcussen]

I would have to understand why I added this, it was for a reason. So I disagree with removing it.

[hishamco]

So I disagree with removing it.

Take your time to check this, I notice that the DateNode that you added is based on DateTime which makes issues with offset, now I need to asked you what are the reasons for adding this ;)

[Skrypt]

This method implies that the datetime is passed as a UtcDateTime. Just add comments on the method. And you could also add an if on the constructor to verify that the datetime is of UtcKind.

[hishamco]

@Skrypt could you please elaborate?

[Skrypt]

What I'm saying is that if DateNode2 uses a DateTime as a parameter and that this DateTime should always be as UTC then you can also validate it inside the DateTime2 constructor instead to keep what we had. If we remove DateNode2 then we need to keep it and mark it as Obsolete instead. But, I assume here that this is used only by the AuditTrail so the changes you made are fine. So, up to @deanmarcussen to illustrate his point.

[hishamco]

I'm still waiting for @deanmarcussen to clarify why DateTime2 is here, it's fine to mark it as obsolete for now

[hishamco]

@Skrypt can we revise this PR while Dean is absent for awhile. @Piedone your though while Lombiq in charge of creating this module

[Piedone]

I checked this out but really have nothing to add.

[Skrypt]

You will need to explain to me what this piece of code does because I have no idea as of now. I would need to analyze the Audit Trail module to understand it better. But here, we have 2 classes that have either a DatetimeOffset or a Datetime as a parameter to differentiate them. Normally, you should use a different constructor on the same class for that matter. But at the same time, the information that no one gave me is about if it is necessary to have a UTC DateTime here. Because then, the class that has a DateTime parameter would need to make sure that the DateTime passed is of UtcKind.

https://docs.microsoft.com/en-us/dotnet/api/system.datetimekind?view=net-6.0

Then, why there are 2 different classes for doing this is what needs to be defined before anything else.

[hishamco]

Then, why there are 2 different classes for doing this is what needs to be defined before anything else.

That's why I waiting for the Dean from awhile, please check his comment here

[deanmarcussen]

Then, why there are 2 different classes for doing this is what needs to be defined before anything else.

That's why I waiting for the Dean from awhile, please check his comment here

I made it quite clear this was not a good pr. Please do not remove the second date class, it is there for a reason.

[hishamco]

I'm stilll waiting for that reason ;) we could check where is it used or fix uncommented tests in the way you prefer

[Skrypt]

Unless it is an issue for someone "which I would really be surprised" then I suggest we leave it as it is for now and come back later if it becomes a need. Else, we are just optimizing code for optimizing it while potentially breaking implementations.

[Piedone]

You can rename DateNode to DateTimeOffsetNode and DateNode2 to e.g. DateTimeNode if you're really inclined to, but that's about what I think would be useful here, apart from fixing the test.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[Piedone]

Superseded by #15297.