You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "jbertram (via GitHub)" <gi...@apache.org> on 2024/06/04 21:18:46 UTC
[PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
jbertram opened a new pull request, #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959
This commit does the following:
- deprecate the verbosely named `toSimpleString` static factory methods
- add `of` static factory methods for all the ctors
- replace any uses of the normal ctors with the `of` counterparts
This makes the code more concise and readable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2150845722
We can merge it I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2149585374
Bigger 'SimpleString API simplication': deletion? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2152810047
I rebased and changed as many of the remaining references to deprecated methods that I could. There are a handful of references to deprecated methods in the compatibility tests, but those must remain due to the fact that those tests are actually using older versions that don't have the new methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#discussion_r1626641094
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java:
##########
@@ -79,6 +96,8 @@ public static SimpleString toSimpleString(final String string, StringSimpleStrin
public SimpleString(final String string) {
int len = string.length();
+ String.valueOf(1);
Review Comment:
Whoops. That was left in by mistake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2151907691
The change should probably be rebased to current main, which has already added new use of the now-deprecated bits that this misses switching over.
From doing a grep, there also looks to remain a number of other existing uses of toSimpleString that perhaps should be updated also.
Feels like we should be unit testing any otherwise-unused deprecated bits, but it doesnt appear like either of the two SimpleStringTest classes do so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2150593808
> > I don't really like the name "of" but that's just my personal taste probably... (of kind of kicks my OCD.. being a preposition, I feel like of what? :)
> > but as I said.. that's my personal preference only...
> > but I don't want to get in the way.. I will get used to it :) and I'm +1 to merge it.. and you should probably merge it soon before you have to rebase it.
> > +1 from me.
>
> I presume it comes from the trend of Map.of(..) etc factory helpers. I dont mind it, though I personally wouldnt have changed it here given I'd rather SimpleString vanish, along with all the realted conversions and the byte doubling etc etc that come with it.
Agreed with Robbie on the name, it matches some Collections APIs etc and isn't hard to grok plus its just less verbose. But I'd also like to see an effort towards removing the SimpleString to simplify working with the broker internals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2150668986
As far as I'm aware, the "of" naming convention was introduced in 2008 in the second edition of Joshua Block's _Effective Java_. He identified it as a way to draw attention to static factories and adhere to common naming conventions. It is a more concise version of `valueOf` which is also used prolifically throughout the Java API. It's an industry standard at this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#discussion_r1628281605
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java:
##########
@@ -56,20 +56,37 @@ public final class SimpleString implements CharSequence, Serializable, Comparabl
* @param string String used to instantiate a SimpleString.
* @return A new SimpleString
*/
- public static SimpleString toSimpleString(final String string) {
+ public static SimpleString of(final String string) {
if (string == null) {
return null;
}
return new SimpleString(string);
}
- public static SimpleString toSimpleString(final String string, StringSimpleStringPool pool) {
+ public static SimpleString of(final String string, StringSimpleStringPool pool) {
if (pool == null) {
- return toSimpleString(string);
+ return of(string);
}
return pool.getOrCreate(string);
}
+ public static SimpleString of(final byte[] data) {
+ return new SimpleString(data);
+ }
+
+ public static SimpleString of(final char c) {
+ return new SimpleString(c);
+ }
+
+ @Deprecated(forRemoval = true)
+ public static SimpleString toSimpleString(final String string) {
+ return of(string);
+ }
+
+ @Deprecated(forRemoval = true)
+ public static SimpleString toSimpleString(final String string, StringSimpleStringPool pool) {
+ return of(string, pool);
+ }
Review Comment:
I fleshed out the JavaDoc on both the old and new methods as well as deprecating the constructors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2150579148
> I don't really like the name "of" but that's just my personal taste probably... (of kind of kicks my OCD.. being a preposition, I feel like of what? :)
>
> but as I said.. that's my personal preference only...
>
> but I don't want to get in the way.. I will get used to it :) and I'm +1 to merge it.. and you should probably merge it soon before you have to rebase it.
>
> +1 from me.
I presume it comes from the trend of Map.of(..) etc factory helpers. I dont mind it, though I personally wouldnt have changed it here given I'd rather SimpleString vanish, along with all the realted conversions and the byte doubling etc etc that come with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#discussion_r1627254132
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java:
##########
@@ -56,20 +56,37 @@ public final class SimpleString implements CharSequence, Serializable, Comparabl
* @param string String used to instantiate a SimpleString.
* @return A new SimpleString
*/
- public static SimpleString toSimpleString(final String string) {
+ public static SimpleString of(final String string) {
if (string == null) {
return null;
}
return new SimpleString(string);
}
- public static SimpleString toSimpleString(final String string, StringSimpleStringPool pool) {
+ public static SimpleString of(final String string, StringSimpleStringPool pool) {
if (pool == null) {
- return toSimpleString(string);
+ return of(string);
}
return pool.getOrCreate(string);
}
+ public static SimpleString of(final byte[] data) {
+ return new SimpleString(data);
+ }
+
+ public static SimpleString of(final char c) {
+ return new SimpleString(c);
+ }
+
+ @Deprecated(forRemoval = true)
+ public static SimpleString toSimpleString(final String string) {
+ return of(string);
+ }
+
+ @Deprecated(forRemoval = true)
+ public static SimpleString toSimpleString(final String string, StringSimpleStringPool pool) {
+ return of(string, pool);
+ }
Review Comment:
In this type of change its typical to have javadoc @ deprecated as well, indicating the replacement that should be used instead. E.g here the constructor is not deprecated (related question: should it also be?), so someone might notice the deprecation and switch to the constructor instead rather than the new 'of'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2150554643
I don't really like the name "of" but that's just my personal taste probably... (of kind of kicks my OCD.. being a preposition, I feel like of what? :)
but as I said.. that's my personal preference only...
but I don't want to get in the way.. I will get used to it :) and I'm +1 to merge it.. and you should probably merge it soon before you have to rebase it.
+1 from me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2150671313
That said, if we can remove `SimpleString` completely I'm good with that too. Of course, this PR is not mutually exclusive with that objective. Consider it a stop-gap on the way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2154566142
Same general review comment as last time. There are still several references to toSimpleString (outside compatibility tests). There is now-unused deprecated stuff that isn't tested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#discussion_r1626637601
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/SimpleString.java:
##########
@@ -79,6 +96,8 @@ public static SimpleString toSimpleString(final String string, StringSimpleStrin
public SimpleString(final String string) {
int len = string.length();
+ String.valueOf(1);
Review Comment:
What is this for in this context?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2150545961
I wouldn't mind seeing SimpleString gone. Change the packets in Core to send char[] ... and be gone!
But that would require a lot of testing.. especially around the pools on reading?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959#issuecomment-2152869890
lets merge and review? otherwise you will keep rebasing forever
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-4796 simplify SimpleString API [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic merged PR #4959:
URL: https://github.com/apache/activemq-artemis/pull/4959
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact