-
Notifications
You must be signed in to change notification settings - Fork 237
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
TEXT-80: Fixed confusing StrLookup API #44
Conversation
@britter , please review and accept this PR. Travis build on this is because previous commit in master , by merging this won't make any issue in master. Tested on local before creating PR. With this comment we can test if activity goes to dev ml. |
LGTM, @chtompki what do you think? |
Would this imply a 2.0 release because of BC compatibility? |
Hi All, Why is it confusing? Why is the patch better? You get the idea, you have to make a point that what you propose is better beyond changing source. Gary |
BTW, this will break source compatibility for certain. |
Hi @garydgregory , Its already discussed here LANG-564 and TEXT-80 and all seems agree on changing the code, according to discussion code proposed in this PR will improve the API. As this is moved from commons lang, version of |
The change is generally all right with me, but we can't release this until a 2.X release though because of signature changes. |
@chtompki did you run chirr manually? Because it was checkstyle which caused the Travis build to fail (trailing white spaces). I think this change should not break BC. @ameyjadiye can you please fix the checkstyle errors and push again? |
Hi @britter, there is no issue even in checkstyle with my changes, above Travis build failed because there was some trailing space issue in master and which you have already fixed. merging this will pass everything clean OR you want me to do some dummy commit so Travis will trigger again and clear error on this PR ? I don't think that's needed though. |
@ameyjadiye the best would be to rebase your branch against master an dann do a force push. You can do it like this (if you have configured this repository as remote with name upstream): git checkout master
git fetch upstream
git merge upstream/master
git checkout TEXT-80
git rebase master
git push -f This will bring your branch up to date with master and trigger a new CI build. |
@britter if I declare a class package com.rt;
import org.apache.commons.text.StrLookup;
public class TextTester extends StrLookup<CharSequence> {
public String lookup(String key) {
return null;
}
} on commons-text:1.1, and up-version into these changes, I get compiler errors. To me, that implies that we must do a major version release to accommodate them. |
@chtompki this indicates a source incompatible release. Binary compatible means that you can swap out the 1.1 jar with the 1.2 jar without a recompile. Would that work? I can't recall our policy for source incompatible changes. But I think for example adding new methods to interfaces has been okay in the past. This is also source incompatible (recompilation fails) but binary compatible (swapping out one jar with another works). |
@britter - I would have to test it out, but fair point. It feels like we should always maintain source and binary backwards compatibility for minor version updates. But, if the policy is different from that feeling, then I'm not opposed to the changes. @garydgregory - What is the policy on source incompatible changes in minor version updates? |
Lets park this for 2.X release. |
I'm closing this as |
No description provided.