Rework Faker::Time::between #1417

pjohnmeyer · 2018-10-16T16:31:23Z

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 · 2018-10-16T16:47:32Z

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 · 2018-10-16T16:48:02Z

~~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.

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.

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.

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

pjohnmeyer · 2018-10-16T17:23:15Z

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 · 2019-07-28T19:53:11Z

test/test_zh_tw_locale.rb

    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


I like the normalizations

vbrazo · 2019-07-28T19:56:06Z

doc/time.md

+# 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"


Could you add keyword arguments for the methods?

…o rework-time-between

* 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

pjohnmeyer changed the base branch from master to v2 October 16, 2018 16:31

pjohnmeyer added 4 commits October 16, 2018 11:59

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.

Improve coverage of Faker::Time docs

960f943

pjohnmeyer force-pushed the rework-time-between branch from 65f0c6b to 960f943 Compare October 16, 2018 17:00

vbrazo assigned pjohnmeyer Oct 16, 2018

vbrazo added the 💡 Issue: Feature Request label Oct 16, 2018

vbrazo mentioned this pull request Nov 11, 2018

Version 2.0 #1356

Closed

vbrazo mentioned this pull request Jul 28, 2019

Preparing for v2 #1651

Merged

vbrazo requested changes Jul 28, 2019

View reviewed changes

vbrazo closed this Jul 28, 2019

vbrazo reopened this Jul 28, 2019

vbrazo added 6 commits July 28, 2019 13:22

Fix merge issues

a19cfce

Update docs

dd1bddf

Add keyword args for Faker::Time

62831ea

Delete time.md

b61258f

Docs

01237b4

Merge branch 'rework-time-between' of github.com:pjohnmeyer/faker int…

2bea673

…o rework-time-between

vbrazo approved these changes Jul 28, 2019

View reviewed changes

vbrazo merged commit 40ddc88 into faker-ruby:v2 Jul 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Faker::Time::between #1417

Rework Faker::Time::between #1417

pjohnmeyer commented Oct 16, 2018

pjohnmeyer commented Oct 16, 2018

pjohnmeyer commented Oct 16, 2018 •

edited

Loading

pjohnmeyer commented Oct 16, 2018

vbrazo Jul 28, 2019

vbrazo Jul 28, 2019

Rework Faker::Time::between #1417

Rework Faker::Time::between #1417

Conversation

pjohnmeyer commented Oct 16, 2018

pjohnmeyer commented Oct 16, 2018

pjohnmeyer commented Oct 16, 2018 • edited Loading

pjohnmeyer commented Oct 16, 2018

vbrazo Jul 28, 2019

Choose a reason for hiding this comment

vbrazo Jul 28, 2019

Choose a reason for hiding this comment

pjohnmeyer commented Oct 16, 2018 •

edited

Loading