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

Add Faker::Date.day_of_week_between #2713

Merged
merged 1 commit into from
Aug 5, 2023
Merged

Conversation

aramvisser
Copy link
Contributor

Summary

This pull requests adds a day_of_week_between method to Faker::Date. It works similar to the existing Faker::Date::between method, but you can ask for only specific days of the week.

For example Faker::Date.day_of_week_between(day: [:saturday, :sunday], from: 1.year.ago, to: Date.today) will produce a random Saturday or Sunday in the last year.

Possible Issues

It's possible that the given date range doesn't include a specific day of week. For example, if the date range is from Monday to Friday and the day of the week is supposed to be a Saturday, that won't work.

Right now it will return a Saturday outside the date range. I'm not familiar enough with Faker how that should be handled. Should that simply return a date outside the range or raise an error?

Other Information

I'm using this generator to create bookings for an app for a company that only accepts bookings on certain weekdays.

doc/default/date.md Outdated Show resolved Hide resolved
Copy link
Contributor

@stefannibrasil stefannibrasil left a comment

Choose a reason for hiding this comment

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

the only issue that I mentioned before is if there is no valid day of the week inside the date range. I'm not sure to just return a date outside the range or raise an error about impossible days.

Hum, good question. If the result is outside the date range, it would be misleading. If it returns an error, since the result is generated randomly, it would not be a good UX to check the calendar for a "valid" date. I'm not sure what the solution is. I can think of is telling users what to do instead. For example: "No date generated between the date range. Try adding a wider range, or a different from or to". What do you think?

lib/faker/default/date.rb Outdated Show resolved Hide resolved
lib/faker/default/date.rb Outdated Show resolved Hide resolved
lib/faker/default/date.rb Outdated Show resolved Hide resolved
lib/faker/default/date.rb Show resolved Hide resolved
lib/faker/default/date.rb Outdated Show resolved Hide resolved
lib/faker/default/date.rb Outdated Show resolved Hide resolved
doc/default/date.md Outdated Show resolved Hide resolved
@aramvisser
Copy link
Contributor Author

...I can think of is telling users what to do instead. For example: "No date generated between the date range. Try adding a wider range, or a different from or to". What do you think?

I agree that that is probably best instead of generating a date outside the range. So now if you ask for a day of the week that is outside the date range, it raises an ArgumentError with the following message:

There is no Friday between 2012-01-01 and 2012-01-03. Increase the from/to date range or choose a different day of the week.

Copy link
Contributor

@stefannibrasil stefannibrasil left a comment

Choose a reason for hiding this comment

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

Looking good! Just left one suggestion for using a test helper.

from = Date.parse('2012-01-01')
to = Date.parse('2012-02-01')

100.times do
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a deterministically_verify test helper that is handy for this test.

test/faker/default/test_faker_date.rb Show resolved Hide resolved
@aramvisser
Copy link
Contributor Author

@stefannibrasil I updated the test to use the deterministically_verify helper.

from = Date.parse('2012-01-01')
to = Date.parse('2012-02-01')

deterministically_verify -> { @tester.on_day_of_week_between(day: days, from: from, to: to) } do |date|
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@stefannibrasil stefannibrasil merged commit d0ebdbe into faker-ruby:main Aug 5, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants