search

bchavez / Bogus

:card_index: A simple fake data generator for C#, F#, and VB.NET. Based on and ported from the famed faker.js.
Other
8.73k stars 496 forks source link

Reference date #492

Closed garcipat closed 12 months ago

garcipat commented 1 year ago

The SystemClock is static and this is problematic if you have different reference dates. I wanted to provide the option to set the reference date on a faker as with the localization. The default is like before, nothing changes, therefore there should be no breaking change.

Example:

var f = new Faker();
f.ReferenceDate = new DateTime(2023, 12, 30, 12, 30, 0);
304NotModified commented 1 year ago

Yes! Great work!

"The SystemClock is static and this is problematic" is an understatement! Is really making things so complicated.

garcipat commented 1 year ago

@bchavez I also don't know if on GitHub people resolve the covnersations after an issue has been solved or I wait untilf the person mentioning it is resolves it himself. I started contributing just recently.

304NotModified commented 1 year ago

@bchavez I also don't know if on GitHub people resolve the covnersations after an issue has been solved or I wait untilf the person mentioning it is resolves it himself. I started contributing just recently.

If it's clear that is has been fixed, it's imo preferred that you press resolve:)

garcipat commented 1 year ago

@bchavez I also don't know if on GitHub people resolve the covnersations after an issue has been solved or I wait untilf the person mentioning it is resolves it himself. I started contributing just recently.

If it's clear that is has been fixed, it's imo preferred that you press resolve:)

Thanks for the clarification :)

garcipat commented 1 year ago

Hello there; hope your days are going well.

Upon further review, I think there's a bug with this PR because the changes missed the Date.net60.cs .NET 6 specific TimeOnly/DateOnly methods which still read from SystemClock() that should have read from GetDate().

Alternatively, I put together PR #500 which fixes the problem and also plumbs the local time ref to Faker<T> and other places where it's probably important API / consistency / ergonomic-wise.

Please post some thoughts on #500.

And for this PR I'm marking this PR as a review block because of the missed SystemClock() bugs in the Date.net60.cs file and until we discuss more.

Let me know your thoughts. Thanks.

Good catch. I will check where the SystemClock is used in egneral and observe if there is any other place I maybe missed.

I renamed the GetDate to Now and used this one everywhere SystemClock was used. The reference date will always be respected then, also in the .net6 methods.

Let me know if you don't like the exposed method. Its also possible to make it private for the partial extensions, but not for the population of the "Person"

304NotModified commented 1 year ago

@garcipat @bchavez Thanks for all the work!

I'm now a bit confused. What is the latest PR of this feature? This one or #500? Or aren't those the same?

garcipat commented 1 year ago

@garcipat @bchavez Thanks for all the work!

I'm now a bit confused. What is the latest PR of this feature? This one or #500? Or aren't those the same?

@304NotModified He extended on my idea and created his own branch in the main repo. I now don't know what to do with this one. I can't help or do anything now 😅

Should I close the PR?

I really want to help further but can't now.

bchavez commented 12 months ago

@garcipat @304NotModified . No worries, I'll make a decision as soon as I review the most recent changes in this 492 PR.

garcipat commented 12 months ago

@garcipat @304NotModified . No worries, I'll make a decision as soon as I review the most recent changes in this 492 PR.

Alright. Thank you.

Or tell me if. Should take over your proposal and add the tests for the new cases.

bchavez commented 12 months ago

Alright, I think we'll go with PR #500 and put our energies there now that things are starting to take shape.

garcipat commented 12 months ago

Alright, I think we'll go with PR #500 and put our energies there now that things are starting to take shape.

But I will then take over to #500 what you already did there. Naming and such.