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.

Sign up for GitHub

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

Jump to bottom

Rework Faker::Time::between #1417

Merged
merged 10 commits into from
Jul 28, 2019
Merged

Rework Faker::Time::between #1417

merged 10 commits into from
Jul 28, 2019

Conversation

pjohnmeyer
Copy link
Contributor

This contains API breaking changes that will require a major version change. See #1356.

Per the discussion on #1280, this pull removes the period argument from Faker::Time::between, making it behave more like Faker::Date::between. It keeps the old functionality available by adding Faker::Time::between_dates, although the logic is now a little simpler due to the elimination of the special case period, :between.

It also removes the inheritance relationship between Faker::Time and Faker::Date.

Resolves #1280

@pjohnmeyer pjohnmeyer changed the base branch from master to v2 October 16, 2018 16:31
@pjohnmeyer
Copy link
Contributor Author

One thing I will note having worked on this; I wonder if it might be "better" to construct a module or two of common functions to share, and then use those modules in Faker::Date, Faker::Time, possible a new Faker::DateTime, and possibly even a Faker::TimeWithZone that is only available if ActiveSupport::TimeWithZone is available. I could imagine something like the below that might be able to simplify the code and make it easy to add more time types.

module Faker
  module DateTimeCommonInterface
    def between
    def forward
    def backward
    # ... other common methods ...
  end

  module TimeCommonInterface
    def between_dates
  end

  class Date < Faker::Base
    include DateTimeCommonInterface
    def get_as_object # returns a Date object
    # ... other required utility methods ... 
  end

  class Time < Faker::Base
    include DateTimeCommonInterface
    include TimeCommonInterface

    def get_as_object # returns a Time object
    # ... other required utility methods ...
  end
end

I didn't want to pollute this PR by going down this road "while I was at it", but I wanted to write down these thoughts in case it's something that may be of value to pursue.

@pjohnmeyer
Copy link
Contributor Author

pjohnmeyer commented Oct 16, 2018 •
edited
Loading

Hmmm, don't know why those tests are failing as they should definitely have nothing to do with changes to Faker::Time. I'll take a look.

Looks like this was fixed in #1411 and I had forgotten to rebase before creating the pull request.

@pjohnmeyer
Add Faker::Time.between_dates, remove period from between
2a40c51 
This is an API breaking change.

Before this change, issues such as faker-ruby#1280 existed due to some
confusion about the behavior of Faker::Time.between. In essence,
between could produce values outside of the range of (from..to)
if the range is smaller than the provided period.

After this change, between now behaves more like Faker::Date.between
and as such the period argument has been removed. For the old
behavior, a user should switch to calling between_dates, which has
the same method signature as the old between method. The :between
period is no longer valid.

Documentation still needs to be updated, and additional tests need
to be written.
@pjohnmeyer
Remove inheritance of Faker::Date by Faker::Time
d4eeea1 
This is an API breaking change.

Before this change, Faker::Time re-used some behavior of Faker::Date
by first inheriting from Date, then using `super`. This relationship
was one of convenience, though, and created some odd behavior for
Time. For example, calling Faker::Time.birthday was possible, as
birthday is defined in Faker::Date, but it would unintuitively return
a Date object.

Now, Faker::Time explicitly calls out to Faker::Date methods
everywhere necessary, and the inheritance relationship has been
removed. This does remove Faker::Time::birthday, and
Faker::Time::between_except from the interface - but as these methods
both return Date objects and are not documented in Faker::Time docs,
they were never indicated for direct use anyway.
@pjohnmeyer
Improve Faker::Time.between tests for v2 behavior
d716c8b 
Adds a new test to prevent regression of faker-ruby#1280, and adds return type
checking for new method between_dates.
@pjohnmeyer
Improve coverage of Faker::Time docs
960f943
@pjohnmeyer
Copy link
Contributor Author

P.S. If this pull is accepted, I will create a follow-up pull to master that adds between_dates as an alias to between on v1 so people can start switching to it pre-v2.

@vbrazo vbrazo mentioned this pull request Nov 11, 2018
Closed
@vbrazo vbrazo mentioned this pull request Jul 28, 2019
Merged
vbrazo
vbrazo requested changes Jul 28, 2019
View reviewed changes
test/test_zh_tw_locale.rb Outdated
assert Faker::Name.last_name.is_a? String
assert Faker::Name.first_name.is_a? String
assert Faker::Name.name.is_a? String
assert Faker::Name.name_with_middle.is_a? String
end

def test_university_methods
def test_zh_tw_university_methods
Copy link
Member

Choose a reason for hiding this comment

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

I like the normalizations

doc/time.md Outdated
# Random Stringified time in the past, future, or between dates, formatted to the specified I18n format
Faker::Time.backward(5, :morning, :short) #=> "14 Oct 07:44"
Faker::Time.forward(5, :evening, :long) #=> "October 21, 2018 20:47"
Faker::Time.between_dates(5.days.ago, 5.days.from_now, :afternoon, :default) #=> "Fri, 19 Oct 2018 15:17:46 -0500"
Copy link
Member

Choose a reason for hiding this comment

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

Could you add keyword arguments for the methods?

@vbrazo vbrazo closed this Jul 28, 2019
@vbrazo vbrazo reopened this Jul 28, 2019
@vbrazo vbrazo merged commit 40ddc88 into faker-ruby:v2 Jul 28, 2019
michebble pushed a commit to michebble/faker that referenced this pull request Feb 16, 2020
@pjohnmeyer @michebble
Rework Faker::Time::between (faker-ruby#1417)
438c54e 
* Add Faker::Time.between_dates, remove period from between

This is an API breaking change.

Before this change, issues such as faker-ruby#1280 existed due to some
confusion about the behavior of Faker::Time.between. In essence,
between could produce values outside of the range of (from..to)
if the range is smaller than the provided period.

After this change, between now behaves more like Faker::Date.between
and as such the period argument has been removed. For the old
behavior, a user should switch to calling between_dates, which has
the same method signature as the old between method. The :between
period is no longer valid.

Documentation still needs to be updated, and additional tests need
to be written.

* Remove inheritance of Faker::Date by Faker::Time

This is an API breaking change.

Before this change, Faker::Time re-used some behavior of Faker::Date
by first inheriting from Date, then using `super`. This relationship
was one of convenience, though, and created some odd behavior for
Time. For example, calling Faker::Time.birthday was possible, as
birthday is defined in Faker::Date, but it would unintuitively return
a Date object.

Now, Faker::Time explicitly calls out to Faker::Date methods
everywhere necessary, and the inheritance relationship has been
removed. This does remove Faker::Time::birthday, and
Faker::Time::between_except from the interface - but as these methods
both return Date objects and are not documented in Faker::Time docs,
they were never indicated for direct use anyway.

* Improve Faker::Time.between tests for v2 behavior

Adds a new test to prevent regression of faker-ruby#1280, and adds return type
checking for new method between_dates.

* Improve coverage of Faker::Time docs

* Update docs

* Add keyword args for Faker::Time

* Delete time.md

* Docs
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
@pjohnmeyer @vbrazo
Rework Faker::Time::between (faker-ruby#1417)
920392e 
* Add Faker::Time.between_dates, remove period from between

This is an API breaking change.

Before this change, issues such as faker-ruby#1280 existed due to some
confusion about the behavior of Faker::Time.between. In essence,
between could produce values outside of the range of (from..to)
if the range is smaller than the provided period.

After this change, between now behaves more like Faker::Date.between
and as such the period argument has been removed. For the old
behavior, a user should switch to calling between_dates, which has
the same method signature as the old between method. The :between
period is no longer valid.

Documentation still needs to be updated, and additional tests need
to be written.

* Remove inheritance of Faker::Date by Faker::Time

This is an API breaking change.

Before this change, Faker::Time re-used some behavior of Faker::Date
by first inheriting from Date, then using `super`. This relationship
was one of convenience, though, and created some odd behavior for
Time. For example, calling Faker::Time.birthday was possible, as
birthday is defined in Faker::Date, but it would unintuitively return
a Date object.

Now, Faker::Time explicitly calls out to Faker::Date methods
everywhere necessary, and the inheritance relationship has been
removed. This does remove Faker::Time::birthday, and
Faker::Time::between_except from the interface - but as these methods
both return Date objects and are not documented in Faker::Time docs,
they were never indicated for direct use anyway.

* Improve Faker::Time.between tests for v2 behavior

Adds a new test to prevent regression of faker-ruby#1280, and adds return type
checking for new method between_dates.

* Improve coverage of Faker::Time docs

* Update docs

* Add keyword args for Faker::Time

* Delete time.md

* Docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbrazo vbrazo vbrazo approved these changes

Assignees

@pjohnmeyer pjohnmeyer

Labels
💡 Issue: Feature Request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pjohnmeyer @vbrazo