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

Make weeksOfMonthByDay match weeks from the end of the month too #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cesarizu
Copy link

@cesarizu cesarizu commented Jun 1, 2016

This PR allows the testing against the week of the month by day from the end of the month, for example the last Friday of the month. This is achieved by passing a negative number to the weeksOfMonthByDay matcher.

@Jean-Rashmir
Copy link

Jean-Rashmir commented Sep 5, 2016

Hi cesarizu,
your modification to allow the calculation of last-day of month recurrences works great, however there is a bug related to the daysOfMonth() method as it doesn´t return any recurrences.

I have created a plunkr to illustrate the issue: http://plnkr.co/edit/GUvwbFLAXXwKcGpucP3D?p=preview

(Just exchange the scripts in the head of index.html to see the difference)

@rmckeel
Copy link

rmckeel commented Sep 20, 2016

This is the exact use case I'm interested in - for example the 'last Sunday' of each month from start date to end date. @cesarizu Would love to see a resolve to @Jean-Rashmir 's issue so this can be integrated! Thanks for the great work!

@cesarizu cesarizu force-pushed the negative-weeks-of-month-by-day branch from b5730f0 to 8cae2c4 Compare September 21, 2016 13:30
@cesarizu
Copy link
Author

cesarizu commented Sep 21, 2016

@Jean-Rashmir and @rmckeel thanks for your comments. If you use this PR's branch: negative-weeks-of-month-by-day instead of my repo's master branch the plunkr works as expected.

There's an error with my repo's master branch with another unrelated commit (caching results for efficiency, see #52). I'll take a look at that PR asap.

EDIT: I've fix the other PR, so master is working for the plunkr just fine now.

@rmckeel
Copy link

rmckeel commented Sep 21, 2016

I tested locally with the PR's code yesterday, and it worked exactly as desired, fitting the test cases I could think of and keeping the existing functionality I had built with the official branch.

@cesarizu cesarizu force-pushed the negative-weeks-of-month-by-day branch from 8cae2c4 to cca5518 Compare October 31, 2016 08:18
@cesarizu
Copy link
Author

@c-trimm I've rebased this branch to the latest master, is there anything else missing or anything you'd like me to do to merge this?

@niftylettuce
Copy link
Contributor

Any update here? This is a core bug otherwise

@Stanton
Copy link

Stanton commented Jan 29, 2019

Bump

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.

5 participants