Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update future, past date generators to use tzinfo #1577

Closed
wants to merge 1 commit into from

Conversation

alexei
Copy link
Contributor

@alexei alexei commented Dec 13, 2021

What does this changes

Fixes #1576

What was wrong

Both methods accepted the timezones, but they were ignored.

How this fixes it

I updated the relevant methods to pass the timezone argument to the underlying helpers.

@fcurella
Copy link
Collaborator

Thank you! Could you add some tests for regressions?

@alexei
Copy link
Contributor Author

alexei commented Dec 14, 2021

Could you add some tests for regressions?

@fcurella I'm thinking about freezing time to e.g. 17+00 and checking that future_date() < future_date(offset=+12) holds true and my tests show that would work. However I did a little digging and I'm a little confused about a couple of things and I would like to clear things up before moving any further.

Looking at the code I see a chain like:

future_date
    -> date_between
        -> date_between_dates
            -> date_time_between_dates.date

I found tzinfo still has no effect due to date_between converting the arguments to dates, so the tzinfo gets lost right from the start. Simply replacing calls to _parse_date() with _parse_date_time wouldn't work either, because _parse_date returns dates while _parse_date_time returns timestamps, so to get to the actual date I'd need to first convert to timestamp_to_datetime. That all seems a little backwards.

However, looking at future_datetime I see something very different:

future_datetime
    -> date_time_between

In light of the above, I have a couple of questions:

  1. date_between almost aliases date_between_dates. What is the difference between and what is the need for the two?
  2. date_time_between and date_time_between_dates seem to achieve the same thing, yet have different implementations. What is the difference? Btw, there's also date_time which has it's own way of achieving a date time between 1970 and "now".
  3. Is there a particular reason why _parse_date returns dates and _parse_date_time returns ints?
  4. Why don't the date_* methods simply call their date_time_* equivalents? Is it by design or chance? With this approach it seems everything would simply converge to a handful of methods.
  5. What is your availability to refactoring this module? Or openness to having someone else do it.

@alexei alexei changed the title fix: update future, past date generators to use fix: update future, past date generators to use tzinfo Dec 14, 2021
@fcurella
Copy link
Collaborator

Hi @alexei , thank you for digging into this.

Most of the mess is for historical reasons. Originally, this library was a port of https://github.com/fzaninotto/Faker, which is written in PHP. Sometimes the porting got more literal than it should have.

There is also a lot of hidden complexity when it comes to calculate dates in Windows, especially Win32, but we should have enough test coverage around that to allow us a refactor.

In short: some complexity here is plain debris, but some is there because computers :)

I'd really like it to refactor it, just be aware there might be some surprising bug along the way.

@alexei
Copy link
Contributor Author

alexei commented Jan 6, 2022

Let's continue the conversation in #1591

@alexei alexei closed this Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

future_date doesn't seem to be using the tzinfo argument
2 participants