Re: Review Request 36703: Remove unnecessary uses of Guava Joiner.

2015-07-22 Thread Joshua Cohen 


> On July 22, 2015, 7:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java, line 45
> > 
> >
> > Yeah, big -1 to me on changes like this.  The previous code was clear 
> > and concise.  The patch is a readability regression IMHO with no obvious 
> > upside.

+1 to this -1. The straight Joiner.on -> String.join changes are great, but the 
other changes really obfuscate what's going on.


- Joshua


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


On July 22, 2015, 7:30 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> ---
> 
> (Updated July 22, 2015, 7:30 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Remove unnecessary use of Guava Joiner.
> 
> This replaces a Guava-ism with the JDK8 standard library APIs. Most of the 
> changes to use `String::join` instead of `Joiner.on(...).join(...)` should be 
> noncontroversial; however in some cases the resulting code is more verbose. 
> This is mostly due to the fact that the standard library requires that 
> arguments be instances of `CharSequence`, requiring the caller to map over 
> `Object::toString`. I'd argue minimizing use of Guava-isms when a standard 
> lib alternative exists is preferable in these cases but don't feel strongly 
> about it.
> 
> There are also some opportunistic changes to surrounding code to use lambdas, 
> method references, and the standard `Streams` API instead of `FluentIterable` 
> and `Ordering`.
> 
> `Joiner` is still used in its `withKeyValueSeparator` form, as no standard 
> library alternative exists for it.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 
> 2b7613741a729e7065bbe74690d543b45802c400 
>   src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java 
> b4163435ea337a9976fae2f84850af0320ab9884 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> a5ffa5e95b301e536a84acf02817ea0c080559d0 
>   src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 
> 904dd72dbb9036d8c06353a953bf35b8f04cfdbc 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 
> 45e062d3acbd7b8565bd3773c1e994aae96378e0 
>   src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java 
> e413ad9bdccc329777b3d0764ba8684539956679 
>   src/main/java/org/apache/aurora/scheduler/http/Slaves.java 
> b64e18c4d5278e6cdb2e480043a45e6ec4f87484 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> 63c31eeb293771b6cc0a3e4cc62dd3a94853e727 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java
>  ff8063c050e13b4bffda2661a817fdc023b80867 
>   
> src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 
> 7232f603e21bbc9dbb5e05aedd5e493de519158e 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 92c970c34ca9dc4f052760e5a3d3770a089d9a67 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
> 7c8f8b9b8d7deb082edc0f85a6d3da1536735545 
> 
> Diff: https://reviews.apache.org/r/36703/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 36703: Remove unnecessary uses of Guava Joiner.

2015-07-22 Thread Aurora ReviewBot 

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


Master (6e2bf57) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On July 22, 2015, 7:30 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> ---
> 
> (Updated July 22, 2015, 7:30 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Remove unnecessary use of Guava Joiner.
> 
> This replaces a Guava-ism with the JDK8 standard library APIs. Most of the 
> changes to use `String::join` instead of `Joiner.on(...).join(...)` should be 
> noncontroversial; however in some cases the resulting code is more verbose. 
> This is mostly due to the fact that the standard library requires that 
> arguments be instances of `CharSequence`, requiring the caller to map over 
> `Object::toString`. I'd argue minimizing use of Guava-isms when a standard 
> lib alternative exists is preferable in these cases but don't feel strongly 
> about it.
> 
> There are also some opportunistic changes to surrounding code to use lambdas, 
> method references, and the standard `Streams` API instead of `FluentIterable` 
> and `Ordering`.
> 
> `Joiner` is still used in its `withKeyValueSeparator` form, as no standard 
> library alternative exists for it.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 
> 2b7613741a729e7065bbe74690d543b45802c400 
>   src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java 
> b4163435ea337a9976fae2f84850af0320ab9884 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> a5ffa5e95b301e536a84acf02817ea0c080559d0 
>   src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 
> 904dd72dbb9036d8c06353a953bf35b8f04cfdbc 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 
> 45e062d3acbd7b8565bd3773c1e994aae96378e0 
>   src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java 
> e413ad9bdccc329777b3d0764ba8684539956679 
>   src/main/java/org/apache/aurora/scheduler/http/Slaves.java 
> b64e18c4d5278e6cdb2e480043a45e6ec4f87484 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> 63c31eeb293771b6cc0a3e4cc62dd3a94853e727 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java
>  ff8063c050e13b4bffda2661a817fdc023b80867 
>   
> src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 
> 7232f603e21bbc9dbb5e05aedd5e493de519158e 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 92c970c34ca9dc4f052760e5a3d3770a089d9a67 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
> 7c8f8b9b8d7deb082edc0f85a6d3da1536735545 
> 
> Diff: https://reviews.apache.org/r/36703/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 36703: Remove unnecessary uses of Guava Joiner.

2015-07-22 Thread Bill Farner 

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



src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java (line 45)


Yeah, big -1 to me on changes like this.  The previous code was clear and 
concise.  The patch is a readability regression IMHO with no obvious upside.


- Bill Farner


On July 22, 2015, 7:30 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> ---
> 
> (Updated July 22, 2015, 7:30 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Remove unnecessary use of Guava Joiner.
> 
> This replaces a Guava-ism with the JDK8 standard library APIs. Most of the 
> changes to use `String::join` instead of `Joiner.on(...).join(...)` should be 
> noncontroversial; however in some cases the resulting code is more verbose. 
> This is mostly due to the fact that the standard library requires that 
> arguments be instances of `CharSequence`, requiring the caller to map over 
> `Object::toString`. I'd argue minimizing use of Guava-isms when a standard 
> lib alternative exists is preferable in these cases but don't feel strongly 
> about it.
> 
> There are also some opportunistic changes to surrounding code to use lambdas, 
> method references, and the standard `Streams` API instead of `FluentIterable` 
> and `Ordering`.
> 
> `Joiner` is still used in its `withKeyValueSeparator` form, as no standard 
> library alternative exists for it.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 
> 2b7613741a729e7065bbe74690d543b45802c400 
>   src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java 
> b4163435ea337a9976fae2f84850af0320ab9884 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> a5ffa5e95b301e536a84acf02817ea0c080559d0 
>   src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 
> 904dd72dbb9036d8c06353a953bf35b8f04cfdbc 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 
> 45e062d3acbd7b8565bd3773c1e994aae96378e0 
>   src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java 
> e413ad9bdccc329777b3d0764ba8684539956679 
>   src/main/java/org/apache/aurora/scheduler/http/Slaves.java 
> b64e18c4d5278e6cdb2e480043a45e6ec4f87484 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> 63c31eeb293771b6cc0a3e4cc62dd3a94853e727 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java
>  ff8063c050e13b4bffda2661a817fdc023b80867 
>   
> src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 
> 7232f603e21bbc9dbb5e05aedd5e493de519158e 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 92c970c34ca9dc4f052760e5a3d3770a089d9a67 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
> 7c8f8b9b8d7deb082edc0f85a6d3da1536735545 
> 
> Diff: https://reviews.apache.org/r/36703/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Review Request 36703: Remove unnecessary uses of Guava Joiner.

2015-07-22 Thread Kevin Sweeney 

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

Review request for Aurora, Joshua Cohen and Bill Farner.


Repository: aurora


Description
---

Remove unnecessary use of Guava Joiner.

This replaces a Guava-ism with the JDK8 standard library APIs. Most of the 
changes to use `String::join` instead of `Joiner.on(...).join(...)` should be 
noncontroversial; however in some cases the resulting code is more verbose. 
This is mostly due to the fact that the standard library requires that 
arguments be instances of `CharSequence`, requiring the caller to map over 
`Object::toString`. I'd argue minimizing use of Guava-isms when a standard lib 
alternative exists is preferable in these cases but don't feel strongly about 
it.

There are also some opportunistic changes to surrounding code to use lambdas, 
method references, and the standard `Streams` API instead of `FluentIterable` 
and `Ordering`.

`Joiner` is still used in its `withKeyValueSeparator` form, as no standard 
library alternative exists for it.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 
2b7613741a729e7065bbe74690d543b45802c400 
  src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java 
b4163435ea337a9976fae2f84850af0320ab9884 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
a5ffa5e95b301e536a84acf02817ea0c080559d0 
  src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 
904dd72dbb9036d8c06353a953bf35b8f04cfdbc 
  src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 
45e062d3acbd7b8565bd3773c1e994aae96378e0 
  src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java 
e413ad9bdccc329777b3d0764ba8684539956679 
  src/main/java/org/apache/aurora/scheduler/http/Slaves.java 
b64e18c4d5278e6cdb2e480043a45e6ec4f87484 
  src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
63c31eeb293771b6cc0a3e4cc62dd3a94853e727 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java 
ff8063c050e13b4bffda2661a817fdc023b80867 
  src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 
7232f603e21bbc9dbb5e05aedd5e493de519158e 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
92c970c34ca9dc4f052760e5a3d3770a089d9a67 
  src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
7c8f8b9b8d7deb082edc0f85a6d3da1536735545 

Diff: https://reviews.apache.org/r/36703/diff/


Testing
---

./gradlew -Pq build


Thanks,

Kevin Sweeney