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

feat(schedule): Support month (M) in schedules #4832

Merged
merged 1 commit into from Nov 24, 2019

Conversation

cfra
Copy link
Contributor

@cfra cfra commented Nov 19, 2019

Compare the M part of the schedule parsed by later so that schedules like every Monday of February-April are handled correctly.

Fixes: #4831

@cfra
Copy link
Contributor Author

cfra commented Nov 19, 2019

As this seemed pretty easy, I thought I'd attempt to implement this one myself.

However, it's my first contribution attempt and it seems like I screwed up the implementation in some aspect, seeing that workers/branch/schedule › isScheduledNow(config) › rejects on months of year is failing.

From the coverage, it seems like the schedule is never checked withschedule.M set. However, testing this on a console, it seems like my test rule does result in schedule.M being set:

> parsedSchedule = later.parse.text("of January");
{ schedules: [ { M: [Array] } ], exceptions: [], error: -1 }
> parsedSchedule.schedules[0].M
[ 1 ]
> !!parsedSchedule.schedules[0].M
true

If you find the time, could you be so kind to provide me with a pointer how I can find the cause of this issue?

@rarkins
Copy link
Collaborator

rarkins commented Nov 19, 2019

@cfra I suspect it's because you need to add M to this list:

s => s.d !== undefined || s.D || s.t_a !== undefined || s.t_b

@cfra cfra force-pushed the feature/month-based-schedules branch from 4a62a72 to 65d449c Compare November 20, 2019 15:38
@cfra cfra force-pushed the feature/month-based-schedules branch from 65d449c to ef77046 Compare November 20, 2019 15:39
@cfra
Copy link
Contributor Author

cfra commented Nov 20, 2019

Thank you for spotting this. The tests seem to be fine now.

@@ -49,10 +49,10 @@ export function hasValidSchedule(
}
if (
!parsedSchedule.schedules.some(
s => s.d !== undefined || s.D || s.t_a !== undefined || s.t_b
s => s.M || s.d !== undefined || s.D || s.t_a !== undefined || s.t_b
Copy link
Collaborator

Choose a reason for hiding this comment

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

I forget - and clearly didn't document - why some checks were !== undefined while others were just simple like s.t_b. I don't suppose you looked into this while deciding what to do for s.M ?

Copy link
Contributor Author

@cfra cfra Nov 20, 2019

Choose a reason for hiding this comment

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

I have to admit I fell for cargo-cult here and copied the check applied to s.D without real understanding. 😬

@rarkins rarkins merged commit c7854cd into renovatebot:master Nov 24, 2019
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 19.68.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@cfra cfra deleted the feature/month-based-schedules branch November 25, 2019 08:33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support month (M) in schedules
3 participants