RangeMonthly should deal with whole months

[lallea]

RangeMonthly should deal with whole months, i.e. months_forward=0 should require only complete months, and not the current month. This was the intended behaviour, and is consistent with other Range* tasks.

I have executed the unit tests for RangeMonthly, as well as updated them to reflect the intended behaviour.

[dlstadther]

I agree this makes more sense than what currently exists. And I appreciate the test case additions!

My only (mild) concern is that someone is using RangeMonthly based on a now which is not the first of the month and thus this patch will change their behavior.

So either moving the day of month to the first should be configurable, OR we explicitly call out in the next release that this may be a breaking change for anyone using RangeMonthly on a date which is not the first of the month.

[lallea]

Thanks for the review, and for caring about compatibility. My reasoning is that this is a bug fix, since the current semantics make no sense, and is undesirable for all use cases. In its current state, it will require e.g. up to 2019-02 if executed on 2019-03-01 with months_forward=0. When executed 2019-03-02, it will instead require up to 2019-03. I think that it does not match any use case, and cannot be easily deduced from the documentation nor the implementation - hence accidental behaviour. Since the current behaviour is not useful, I don't think that we should keep it as a configurable option. IMHO, it does make sense to bump the minor release number instead of the patch number for the next release in order to signal the risk of breakage, however. Lars Albertsson Founder, Mimeria www.mimeria.com www.mapflat.com +46 70 7687109

…

On Tue, Mar 5, 2019 at 1:14 PM Dillon Stadther ***@***.***> wrote: I agree this makes more sense than what currently exists. And I appreciate the test case additions! My only (mild) concern is that *someone* is using RangeMonthly based on a now which is not the first of the month and thus this patch will change their behavior. So either moving the day of month to the first should be configurable, OR we explicitly call out in the next release that this may be a breaking change for anyone using RangeMonthly on a date which is not the first of the month. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2666 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC5Smrcq7Mo0TyG7ilBlLKUIFjdP7KFHks5vTl-qgaJpZM4bcfWa> .

[Tarrasch]

Since the current behaviour is not useful, I don't think that we should keep it as a configurable option. IMHO, it does make sense to bump the minor release number instead of the patch number for the next release in order to signal the risk of breakage, however.

Sounds good to me! I think you added this 3 months ago, so it's probably also not going to break for to many users. Since we're "understaffed" I think it's fine to cut maintenance corners if the negative impact is low.

[lallea]

Sounds reasonable to me, thanks. I still think that it is wise to bump the minor number to indicate risk of breakage, however. I have had Luigi patch releases break pipelines in the past. I think that it is better to be generous with major and minor bumps in order to set expectations on compatibility. Thanks for spending time reviewing - much appreciated! Lars Albertsson Founder, Mimeria www.mimeria.com www.mapflat.com +46 70 7687109

…

On Tue, Mar 5, 2019 at 9:27 PM Arash Rouhani ***@***.***> wrote: ***@***.**** approved this pull request. Since the current behaviour is not useful, I don't think that we should keep it as a configurable option. IMHO, it does make sense to bump the minor release number instead of the patch number for the next release in order to signal the risk of breakage, however. Sounds good to me! I think you added this 3 months ago, so it's probably also not going to break for to many users. Since we're "understaffed" I think it's fine to cut maintenance corners if the negative impact is low. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2666 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC5SmnUKOeiHMq3pXYj0J_ywIs21paCCks5vTtM0gaJpZM4bcfWa> .

[Tarrasch]

I still think that it is wise to bump the minor number to indicate risk of breakage, however.

Oh yes. I didn't mean otherwise.

I have had Luigi patch releases break pipelines in the past. I think that it is better to be generous with major and minor bumps in order to set expectations on compatibility.

Totally agree. Sorry for any breakages!

[dlstadther]

Sorry for my silence @lallea

Looks like we agree for this to be merged, given infancy of feature and minimal breakage.

I'll merge. Thanks @lallea !