[GitHub] commons-text issue #44: TEXT-80: Fixed confusing StrLookup API
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-text/pull/44 I'm closing this as `StrLookup` will deprecated and replaced with `StringLookup` in the next release. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user ameyjadiye commented on the issue: https://github.com/apache/commons-text/pull/44 Lets park this for 2.X release. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user coveralls commented on the issue: https://github.com/apache/commons-text/pull/44 [](https://coveralls.io/builds/11887530) Coverage remained the same at 96.653% when pulling **fff3f43d27ed831986b36691eaeb7570eaa94d63 on ameyjadiye:TEXT-80** into **5f498c0f4783d035bfeb77517731c948f8567b1e on apache:master**. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user chtompki commented on the issue: https://github.com/apache/commons-text/pull/44 @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? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user britter commented on the issue: https://github.com/apache/commons-text/pull/44 @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). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user chtompki commented on the issue:
https://github.com/apache/commons-text/pull/44
@britter if I declare a class
```java
package com.rt;
import org.apache.commons.text.StrLookup;
public class TextTester extends StrLookup {
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user britter commented on the issue: https://github.com/apache/commons-text/pull/44 @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): ```shell 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user ameyjadiye commented on the issue: https://github.com/apache/commons-text/pull/44 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user britter commented on the issue: https://github.com/apache/commons-text/pull/44 @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? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user ameyjadiye commented on the issue: https://github.com/apache/commons-text/pull/44 Hi @chtompki , since commons-text is relatively new there are very [low usage](https://mvnrepository.com/artifact/org.apache.commons/commons-text), ```clirr:check``` is also passed whoever is using it and wants to bump version needs minor change in their code, but its ok we can park it till 2.X release. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user chtompki commented on the issue: https://github.com/apache/commons-text/pull/44 The change is generally all right with me, but we can't release this until a 2.X release though because of signature changes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user ameyjadiye commented on the issue: https://github.com/apache/commons-text/pull/44 Hi @garydgregory , Its already discussed here [LANG-564](https://issues.apache.org/jira/browse/LANG-564) and [TEXT-80](https://issues.apache.org/jira/browse/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 ```StrLookup``` will soon be deprecated. so no cross module impact I see for now and yes whoever using text 1.0, 1.1 will need to change code very minimal. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user garydgregory commented on the issue: https://github.com/apache/commons-text/pull/44 BTW, this will break source compatibility for certain. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user garydgregory commented on the issue: https://github.com/apache/commons-text/pull/44 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 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user chtompki commented on the issue: https://github.com/apache/commons-text/pull/44 Would this imply a 2.0 release because of BC compatibility? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user britter commented on the issue: https://github.com/apache/commons-text/pull/44 LGTM, @chtompki what do you think? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-text issue #44: [TEXT-80]: Fixed confusing StrLookup API
Github user ameyjadiye commented on the issue: https://github.com/apache/commons-text/pull/44 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
