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

Should adding a method always trigger MAJOR change? #86

Open
ajant opened this issue May 9, 2016 · 5 comments
Open

Should adding a method always trigger MAJOR change? #86

ajant opened this issue May 9, 2016 · 5 comments

Comments

@ajant
Copy link

ajant commented May 9, 2016

Per V015 it makes sense when a method is being overridden, but if parent class does not contain it or there is no parent class, this should probably be a MINOR change, since there are no backwards compatibility issues. Right?

@nochso
Copy link
Contributor

nochso commented May 9, 2016

Imagine you're using class X. You inherit from X and add a method foo().

Now X gets updated and it suddenly has a method foo($bar)! Your code broke and thus it's MAJOR.

Exceptions are final classes because you can't extend them. However afaik that is not currently implemented as a rule.

@tomzx tomzx added this to the Candidate for next Major milestone May 9, 2016
@tomzx
Copy link
Owner

tomzx commented May 9, 2016

I don't think that the first release will support logic that consider the hierarchy of the scanned classes.

For the moment, it is only analyzing the files that have changes which reduce the amount of time spent analyzing the code. To be able to analyze a change such as the one discussed here, it would have to load the ancestors in order to determine the available methods, which is not something we do at the moment (everything is analyzed through PHP Parser and no classes are actually loaded per se).

I'm adding this to the next major release as it is something I also think would be worth having, but will require a different approach than the currently implemented solution.

@ajant
Copy link
Author

ajant commented May 10, 2016

@nochso I understand what you're saying, but I don't think that should trigger major flag until X is updated in a way you talked about, if even then. Major flag should be raised only when backwards compatibility is broken. Scenario you are talking about, I don't think is about that, but rather about code quality. While that's definitely an important issue, probably it's not something we can expect semver checker library to take care of.

@tomzx sounds good, thanx

@emmetog
Copy link
Contributor

emmetog commented Nov 27, 2017

Hi all, I'm another believer that adding methods should be MINOR (although I admit that technically they could be a breaking change if someone has extended the class and added a new method with the same name, but that's an edge case and from what I've seen in the community (eg symfony's BC promise) it's not considered as breaking BC.

I've opened #93 to change this, feel free to chime in.

@emmetog
Copy link
Contributor

emmetog commented Feb 8, 2018

I've learned here that php-semver-checker can already override any individual rule to be any level you want, so defaulting to the strictest possible interpretation of the semantic versioning rules is probably a good starting point since anyone can relax any particular rule they want to, depending on their use case.

I recommend closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants