#1380 support explicitly set scale of zero for datetime2

[EamonHetherton]

...datetimeoffset and time

[dnfadmin]

All CLA requirements met.

[Wraith2]

I've mentioned my misgivings on the issue so I won't repeat them here.
The implementation looks correct. Can you add test(s)?

[EamonHetherton]

I've two different points to discuss (supporting existing behaviour and tests), so I'll do them in two separate comments.

Regarding the existing behaviour and minimising the impact of a change, I think it is still worth discussing and maybe coming to a consensus. There are three realistic options I see:

1. Change the behaviour as discussed in #1380.

2. Do 1 and create an AppContextSwitch to opt-in to the existing behaviour.
3. Do 1 and create an AppContextSwitch to opt-in to the new behaviour.

Then there is a second dimension of netfx vs netcore. With all the work to merge code bases I suspect there is not a lot of appetite to diverge behavior on the NETFRAMEWORK switch but it's worth asking. It seems anecdotally to me at least that the threshold for breaking changes is higher in netfx than netcore but not sure if that is actually policy or where I'd find out. There could be an option to retain existing behavior on netfx as default with an op-in switch for the correct behaviour and just the correct behaviour on netcore? (the SqlParameter class is not yet common across netfx and netcore)

Your thoughts?

[Wraith2]

(the SqlParameter class is not yet common across netfx and netcore)

There's a PR for it, #1354 but PR's aren't merged in order so whichever gets merged first the other will need to resolve the conflicts.

Then there is a second dimension of netfx vs netcore.

My opinion is that we should minimize the observable differences between netcore and netfx so whatever is done it should be the same. We have plenty of problems between windows and linux implementation at the network layer which it's not obvious to users why they are different, it's just easier for everyone to avoid that sort of situation.

There are three realistic options I see:

My choice would be #2 (Do 1 and create an AppContextSwitch to opt-in to the existing behaviour.) but it's down to the team decision, I just hang around here because I have no hobbies.

[EamonHetherton]

As for tests, I did try :)

The SqlParameter class is sealed and the methods involved here are all internal or private apart from the Scale property itself which already returns whatever value is set so I couldn't write a failing test with the current code.

I followed the callers of GetActualScale to see if I could get to it by proxy and again I was blocked by the SqlCommand class being sealed and relevant methods being internal. Beyond that I was getting to the point of just about having to execute a command with a valid connection and trying to interrogate the produced sql which is pretty far from unit test land.

Removing the sealed attribute would allow me to create a derived class for testing but implications for existing code that may depend on the SqlParameter being sealed are unknown so not comfortable with that approach.

So the simplest approach to being able to test this is to add an "[assembly: InternalsVisibleTo("FunctionalTests")]" attribute. I'll head down that road unless it is likely to get rejected?

[Wraith2]

I wouldn't think there is a sensible way to add a functional test (functional tests don't require a database, manual tests do) because as you've said it requires either surface area changes or internal shenanigans which are better avoided.

You can just add a test case to the ManualTests which writes a value to the database like your original issue report and then verify that the scale has been correctly written can't you?

[JRahnama]

@EamonHetherton currently we are on code freeze due to our next GA release. We will resume back on PRs after release.

[Charlieface]

I concur that this is beyond ridiculous.

99.999% of code that relies on the default just uses AddWithValue or similar.

As a compromise: what about only making this change if the Precision value is also manually set to 0?