Picking last minor release vs. the last release

[ondrejmirtes]

Hi,
I just noticed an oddity in the logic - the tool compares current HEAD against the last minor release, meaning even if there's 2.2.5, it will compare HEAD against 2.2.0.

I think this will lead to not noticing BC breaks between patch versions.

Let's say I add a new method in 2.2.1. Because the method does not exist in 2.2.0, the tool will consider it as a new method in the current HEAD when getting ready to release 2.2.5. Which means that if I add a new required argument between 2.2.4 and 2.2.5, I just caused a BC break for users that started relying on the method meanwhile (between 2.2.1 and 2.2.4).

I know that patch versions are for bugfixes only, but I can imagine a scenario where adding a new method can be interpreted as part of a bugfix...

Is there something I'm not seeing? Thanks!

[Ocramius]

Because the method does not exist in 2.2.0

This should IMO have been where the new API method is introduced?

[Ocramius]

Reading the whole thing, I think it makes sense to compare BC against the latest patch, instead of the latest minor.

The initial idea I had when developing this library is that:

• new features are developed in new minors
• those new minors need to be protected against BC

In fact though, if BC is incrementally maintained between patch releases too, that shouldn't be a problem.

[ondrejmirtes]

Yeah, I guess I'd sleep calmer if HEAD was always compared against the latest release, be it major, minor, or even patch :)

[asgrim]

Maybe make it an option? --compare-against-latest-patch, --compare-against-latest-minor, etc.?

[Ocramius]

Imo `--from` covers that, and git+bash can do the rest

…

On Sat, May 29, 2021, 11:25 James Titcumb ***@***.***> wrote: Maybe make it an option? --compare-against-latest-patch, --compare-against-latest-minor, etc.? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#307 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEHSILG2WNJNPS6Y4I3TQCXH5ANCNFSM45WXCZUQ> .

[Ocramius]

I'm targeting this for 6.0: it's a behavioral change that is worth a BC break.

[Ocramius]

Not going to be able to drag this into 6.0 - removing milestone for now.

[Ocramius]

Looking back at this, I can imagine that:

• roave/backward-compatibility-check is generally used in CI, to verify pull requests
• merging a red PR from a contributor will lead to all subsequent PRs going red, until a new minor release is cut
• the idea is to check BC from patch to patch

I suggest changing the defaults of this library from checking the latest minor release to checking the base of the current PR (which will probably require looking at the current environment variables, probably), or HEAD^1, should that fail.

[asgrim]

merging a red PR from a contributor will lead to all subsequent PRs going red, until a new minor release is cut

Yes, I observed this where I'm using roave/backward-compatibility-check; I figured it was desirable, but I can see why it's not ideal for others.

[Ocramius]

Yup, and also, nothing denies providing a --from= manually, allowing for consumers to always pick their own approach.

[Ocramius]

I removed the milestone for now, since I released 7.0.0 just to get another BC break out of the door, for now.

[michael-rubel]

Hey @Ocramius, can we change this tool to pick the latest release? There's a case, for example, when methods were added to a class by mistake (from a trait) and weren't meant to be used in the class at all. Looking from a clear technical perspective, this is of course a BC break, but looking from practical usage, the methods are meaningless and can be fixed (removed) in a patch.

[Ocramius]

@michael-rubel see #307 (comment)

Feel free to send a patch for the next major.

[michael-rubel]

@Ocramius looks like @ondrejmirtes already done that in his fork. Should I resubmit that as a separate PR?

[Ocramius]

Sure 👍

[michael-rubel]

@Ocramius done in: #687

Not sure the target branch is right, but I haven't found the 8.0.x.

[Ocramius]

I'll change the target branch before merging :)

[Ocramius]

Handled in #687