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

Add support for BYSETPOS (monthly and yearly frequency) #468

Closed
wants to merge 5 commits into from
Closed

Add support for BYSETPOS (monthly and yearly frequency) #468

wants to merge 5 commits into from

Conversation

joshbeckman
Copy link

In August 2016, @NicolasMarlier added BYSETPOS support (#349).

Then, in July 2018, @nehresma added a few small changes to run in modern Ruby and a more modern rspec (#449).

Then, in January 2019, @davidstosik and @k3rni suggested changes to reduce complexity.

This incorporates all the above into a single diff.

@joshbeckman
Add support for BYSETPOS (monthly and yearly frequency)
423faef 
In August 2016, @NicolasMarlier added BYSETPOS support (ice-cube-ruby#349).

Then, in July 2018, @nehresma added a few small changes to run in modern
Ruby and a more modern rspec
(ice-cube-ruby#449).

Then, in January 2019, @davidstosik and @k3rni suggested changes to
reduce complexity.

This incorporates all the above into a single diff.
@joshbeckman
Fix reference to include over range
9ec3b06 
Rubocop recommends cover? instead.
@joshbeckman
Fix reference to start_of_month on Time (it's beginning_of_month)
65dca74
@joshbeckman
Use underlying prev_month/_year for greater ActuveSupport versions
2f26348 
Ref: https://apidock.com/rails/DateAndTime/Calculations/prev_month
@joshbeckman
Fix reference to Hash#except
8d9d2ec 
This is not available within the currently supported Rails versions

Ref: https://apidock.com/rails/Hash/except
@joshbeckman
Copy link
Author

Note that currently, all logic is passing but some builds are failing due to (I believe): tzinfo/tzinfo#84

@davidstosik
Copy link

davidstosik commented Feb 6, 2019 •
edited
Loading

Looks like Travis has other problems, related to the Bundler version. You might want to rebase your branch on the latest master.

@davidstosik
Copy link

davidstosik commented Feb 6, 2019 •
edited
Loading

I worked on the BYSETPOS addition myself too by the way, and here's the state of what I have: davidstosik#1

One of the specs I added actually fails, so I'd be curious to have others' opinion too. (And to see if all the specs I added pass in your implementation.)

@joshbeckman
Copy link
Author

@davidstosik Thanks! I'll have a look at your branch.

I branched off the latest master to add these changes, so I'm also not sure why bundler is complaining.

I'm also trying to work on the to_s representation of the BYSETPOS data.

@joshbeckman
Copy link
Author

I also like your more generic version of BYSETPOS, so I'll close this in favor of your branch.

@joshbeckman joshbeckman closed this Feb 6, 2019
@davidstosik davidstosik mentioned this pull request Feb 8, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joshbeckman @davidstosik