Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add properties Now and Today #30682

Merged
merged 1 commit into from
Jul 28, 2018
Merged

Conversation

EdwinEngelen
Copy link
Contributor

Newly added module DateAndTime with read-only properties Now and Today. Implementation is the same as in netfx.

Property Today is writable in netfx and uses kernel32.dll. That's not possible in corefx, thus property is read-only.

I tried to add tests, but experienced two problems:

  • Faking DateTime.Now requires the use of ShimsContext. Then it's no longer safe to run tests in parallel, which is what probably happens.
  • I couldn't find out how to add the fakes assembly, because I couldn't see the netstandard assembly in the reference list.

@stephentoub
Copy link
Member

That's not possible in corefx

Why? At least on Windows that should be doable. I expect there's an equivalent that could be done on other platforms, like using settimeofday, but if there really isn't any way it could be implemented on unix it should still be added but then throw PlatformNotSupportedException.

@stephentoub
Copy link
Member

I tried to add tests

You should still be able to test it, e.g.

  • Call DateTime.Now and store the result
  • Call DateAndTime.Now and store the result
  • Call DateTime.Now and store the result
  • Validate that the middle result was >= to the first and <= to the second. You can convert these to UTC to do the comparisons.

@EdwinEngelen EdwinEngelen force-pushed the NowAndToday branch 3 times, most recently from 5bcc285 to 0555c43 Compare June 27, 2018 12:17
@EdwinEngelen
Copy link
Contributor Author

  • I've added tests. Are these as proposed?
  • Set date/time: Do we need to implement these now? I think it's okay to add that later. I also don't know how to implement it for other platforms. Throwing PlaformNotSupportedExcepton might not be a good idea, because that results in runtime errors and not in compilation errors.

@stephentoub
Copy link
Member

Do we need to implement these now? I think it's okay to add that later.

If we're adding the properties now, I think we should add them correctly now.

@EdwinEngelen
Copy link
Contributor Author

EdwinEngelen commented Jun 27, 2018

I think we should add them correctly now

Then I'll need some help from someone.

@karelz
Copy link
Member

karelz commented Jul 19, 2018

@OmarTawfik can you please help move the PR forward?

@karelz karelz added this to the 3.0 milestone Jul 19, 2018
@danmoseley
Copy link
Member

@EdwinEngelen could you rebase?

@stephentoub I think it is reasonable to check in just the getters alone as a step along for https://github.com/dotnet/corefx/issues/31181. It is not possible for us to forget the setters as they don't show in the ref, and there is presumably code that only use the getters.

@stephentoub
Copy link
Member

It is not possible for us to forget the setters as they don't show in the ref

Does that mean we have tooling that shows up everything we're supposed to have but are missing, it's currently showing the getters and setters, and once this goes in it'll still show the setters?

@OmarTawfik
Copy link
Contributor

@danmosemsft can you please elaborate on It is not possible for us to forget the setters as they don't show in the ref? what guards do we have in place to make sure the new APIs we add actually match the old ones (other than manual review)?

var today = DateAndTime.Today.ToUniversalTime();
var dateTimeTodayAfter = DateTime.Today.ToUniversalTime();

Assert.InRange(today, dateTimeTodayBefore, dateTimeTodayAfter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

today [](start = 27, length = 5)

maybe also assert that time component is set to zeroes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need that assertion too. Thanks!
I rebased and tried to add the assertion, but somehow VS gives me strange compile errors. System.Object not defined, …
I'm on holiday and don't have time to figure it out right now.

@danmoseley
Copy link
Member

danmoseley commented Jul 20, 2018

Does that mean we have tooling that shows up everything we're supposed to have but are missing, it's currently showing the getters and setters, and once this goes in it'll still show the setters?

Yes, if you count running genapis and windiff, or Apireviewer, when we believe we are done or ready to review where we are at. Something which we would do for any library port, but especially in this case where there was a very selective existing surface area.

what guards do we have in place to make sure the new APIs we add actually match the old ones (other than manual review)?

The build validates our implementation (src) against our contract (ref) in both directions, but there is no automatic verification against .NET Framework surface area. That is what is done by diffing the output of genapis on the binaries.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @EdwinEngelen

@danmoseley danmoseley merged commit dfa1338 into dotnet:master Jul 28, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants