geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jinmei Liao <jil...@pivotal.io>
Subject Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.
Date Wed, 17 May 2017 17:33:43 GMT


> On May 17, 2017, 5:32 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/internal/lang/StringUtils.java
> > Line 29 (original), 29 (patched)
> > <https://reviews.apache.org/r/59323/diff/2/?file=1721981#file1721981line29>
> >
> >     I would not make this class extends apache common's StringUtils. We should always
encourage devleopers to use the apache's String utils, and only for very specific cases, we
would want to use our own.

You can make this change after you commited this. I would imagine this would touch more files.


- Jinmei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/#review175278
-----------------------------------------------------------


On May 16, 2017, 11:25 p.m., Patrick Rhomberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59323/
> -----------------------------------------------------------
> 
> (Updated May 16, 2017, 11:25 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and Swapnil
Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> *   Renamed:
> *   --  EMPTY_STRING -> EMPTY (inherited)
> *   --  toUpperCase  -> upperCase (inherited)
> *   --  toLowerCase  -> lowerCase (inherited)
> *   --  padEnding    -> rightPad (inherited)
> *
> *   Removed:
> *   --  EMPTY_STRING_ARRAY; usage replaced with commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
> *   --  SPACES
> *   --  UTF_8; rare usage replaced with raw string
> *   --  concat; usage replaced with commons.lang.join, refactoring as necessary.
> *   --  getLettersOnly
> *   --  getSpaces
> *   --  truncate
> *   --  valueOf; usage refactored to use defaultString
> *
> *   Refactored
> *   --  defaultIfBlank: previously relied on varargs and could return null.  Usage refactored
to allow inheritance from commons.
> *   --  defaultString(s, EMPTY) refactored to use standard signature defaultString(s)
for consistency throughout codebase.
> *   --  isBlank: usage refactored to resolve discrepancies with commons.lang.isBlank,
which is now inherited.
> *   --  isEmpty: usage refactored to resolve discrepancies with commons.lang.isEmpty,
which is now inherited.
> *
> *   Code Cleanup:
> *   --  Many uses of !isBlank -> isNotBlank
> *   --  Changes suggested by Inspections on most touched files.
> *   --     Explicit <T> -> <> when type is inferable
> *   --     while loops operating on iterators converted to for each loops
> *   --     for loops operating on array indices converted to for each loops
> *   --  Various string typos corrected.
> *   --  isEmpty(s.trim()) -> isBlank(s)
> *   --  s.trim().isEmpty() -> isEmpty(s)
> *   --  Removed some instances of 'dead' code
> *
> *   Qualitative Changes:
> *   --  The following functions now throw an error when called with a null string input:
> *   --  *  LocatorLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setHostnameForClients
> *
> *   Notes:
> *   --  StringUtils.wraps may be inherited from Apache Commons when the dependency is
updated.
> *   --  AbstractLauncher.getMember has the documented behavior of returning null when
both MemberName and ID are blank.  Is this the best behavior for this method?
> 
> ======
> 
> I'm going through the diff myself now and noticing a lot of refactorings that happen
in dead code.  I'm going back to cull the dead code entirely, but wanted to go ahead and get
this diff posted.
> 
> 
> Diffs
> -----
> 
>   geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
5277e57 
>   geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
0554f69 
>   geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPITestBase.java
f2e90a4 
>   geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
d531cc1 
>   geode-core/src/main/java/org/apache/geode/cache/query/internal/AttributeDescriptor.java
f40ab3e 
>   geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
4e52a67 
>   geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java feba893

>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 641e009

>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java b2d0151 
>   geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java
ad5e04d 
>   geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
4bf010b 
>   geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java 78b2944

>   geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java
af3cb5d 
>   geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
55e3542 
>   geode-core/src/main/java/org/apache/geode/internal/lang/StringUtils.java 499d546 
>   geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 3485ede 
>   geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java
629740c 
>   geode-core/src/main/java/org/apache/geode/internal/process/signal/Signal.java faab4ed

>   geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java
8366930 
>   geode-core/src/main/java/org/apache/geode/internal/util/ArrayUtils.java 6f1c7cc 
>   geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java c1a1952 
>   geode-core/src/main/java/org/apache/geode/management/internal/AgentUtil.java ba15508

>   geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 7c26297

>   geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
62310e8 
>   geode-core/src/main/java/org/apache/geode/management/internal/RestAgent.java 837e815

>   geode-core/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemBridge.java
ef643ac 
>   geode-core/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemMBean.java
a87b366 
>   geode-core/src/main/java/org/apache/geode/management/internal/beans/ManagementAdapter.java
7dce602 
>   geode-core/src/main/java/org/apache/geode/management/internal/beans/QueryDataFunction.java
f701d29 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java
5dfc1b8 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
5b0651e 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/IndexCommands.java
9110a1a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
101bae4 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
c9e79b0 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewaySenderIdConverter.java
59851da 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/IndexDetails.java
64bb512 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java
4383044 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java
9dbf59c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java
9bd2bf9 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TableBuilder.java
da3c4ae 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java
7c80e0d 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java
e9d183e 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java
4410fea 
>   geode-core/src/main/java/org/apache/geode/management/internal/configuration/domain/XmlEntity.java
be74e84 
>   geode-core/src/main/java/org/apache/geode/management/internal/configuration/messages/ConfigurationRequest.java
837d99e 
>   geode-core/src/main/java/org/apache/geode/management/internal/configuration/messages/ConfigurationResponse.java
3248c98 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/AbstractCommandsController.java
0b64a44 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ConfigCommandsController.java
f468c65 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
3a8ed82 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DeployCommandsController.java
fe5be62 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DiskStoreCommandsController.java
661340c 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DurableClientCommandsController.java
1d718b4 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
604bdee 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/FunctionCommandsController.java
77cfb40 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MiscellaneousCommandsController.java
d19aee1 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/PdxCommandsController.java
c68ee35 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/QueueCommandsController.java
396726e 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/RegionCommandsController.java
e503e56 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java
0ecb77f 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/WanCommandsController.java
fa5aa57 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/domain/Link.java
cef8cab 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/domain/LinkIndex.java
7d5bb37 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/http/HttpHeader.java
ee7e932 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvoker.java
eeedf40 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/util/UriUtils.java
b83064e 
>   geode-core/src/test/java/org/apache/geode/cache/client/internal/LocatorTestBase.java
c3b349a 
>   geode-core/src/test/java/org/apache/geode/distributed/AbstractLauncherTest.java 62d4bdd

>   geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherIntegrationTest.java
5b53c6a 
>   geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java 06d6054

>   geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherIntegrationTest.java
7bd7462 
>   geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java f5d6271

>   geode-core/src/test/java/org/apache/geode/internal/cache/execute/Bug51193DUnitTest.java
0dfbe6c 
>   geode-core/src/test/java/org/apache/geode/internal/lang/ClassUtilsJUnitTest.java 3ec3b06

>   geode-core/src/test/java/org/apache/geode/internal/lang/StringUtilsJUnitTest.java f3e0abe

>   geode-core/src/test/java/org/apache/geode/internal/util/CollectionUtilsJUnitTest.java
36be8b8 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupportJUnitTest.java
91a59f8 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListIndexCommandDUnitTest.java
8bf1c43 
>   geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfig.java
fc920c4 
>   geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java
b2be12a 
>   geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsJUnitTest.java
d4dca4a 
>   geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/PropMockDataUpdater.java
6ee5b53 
>   geode-spark-connector/geode-functions/src/main/java/org/apache/geode/spark/connector/internal/geodefunctions/RetrieveRegionFunction.java
096e4d5 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/AbstractWebTestCase.java
eac0b8d 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
37ec508 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvokerJUnitTest.java
c69013b 
> 
> 
> Diff: https://reviews.apache.org/r/59323/diff/2/
> 
> 
> Testing
> -------
> 
> Precheckin running.  Tests and LegacyTests already returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message