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] workday?, weekday? to return bool instead of true or nil #208

Closed
wants to merge 1 commit into from

Conversation

3150
Copy link

@3150 3150 commented May 26, 2021

Hello. I'm Brown.
It seems that workday?, weekday? return 'true' or 'nil' instead of false.

irb(main):009:0> RUBY_VERSION
=> "2.6.6"
irb(main):010:0> today = Date.current
=> Wed, 26 May 2021
irb(main):011:0> sunday = today + 3.days
=> Sat, 29 May 2021
irb(main):012:0> today.weekday?
=> true
irb(main):013:0> today.workday?
=> true

irb(main):015:0> sunday.weekday?
=> nil
irb(main):016:0> sunday.workday?
=> nil

It is because {SortedSet}.include?() returns true or nil

irb(main):020:0> BusinessTime::Config.weekdays
=> #<SortedSet: {1, 2, 3, 4, 5}>

irb(main):017:0> a = SortedSet.new([1,2])
=> #<SortedSet: {1, 2}>
irb(main):018:0> a.include?(2)
=> true
irb(main):019:0> a.include?(3)
=> nil

How about changing these two methods to return true or false like this?

irb(main):003:0> today = Date.current
=> Wed, 26 May 2021
irb(main):004:0> sunday = today + 3.days
=> Sat, 29 May 2021
irb(main):005:0> today.weekday?
=> true
irb(main):006:0> today.workday?
=> true
irb(main):007:0> sunday.weekday?
=> false
irb(main):008:0> sunday.workday?
=> false

related issue:
knu/sorted_set#2

@pmcnano
Copy link

pmcnano commented Nov 19, 2021

Pushed a PR 🤞 knu/sorted_set#9

@rmm5t
Copy link
Collaborator

rmm5t commented Nov 20, 2021

Thank you for this, but this is not necessary. If sorted_set wants to handle it, great, but it's not appropriate to handle here.

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.

3 participants