building on that, I think the first priority of reviewer time includes.
What is this changing?
Why is jclouds better off with this change?
If feature, who is using this, and for what?
A lot of our reviews are extremely focused on the mechanics of code,
and that makes it possible to skip the first step.. what exactly are
we building here?
-A
On Wed, Oct 8, 2014 at 8:07 PM, Andrew Gaul <g...@apache.org> wrote:
> Not picking on this pull request in particular, but I find stylistic
> feedback to be a poor use of code reviews. We should use humans to
> identify logic errors, missing tests, and higher-level code quality
> (HashCode vs. byte[], Objects.equals, etc.). We should use machines to
> codify lower-level issues (whitespace, brace placement, etc.) or just
> let it go. Could the reviewers making large numbers of stylistic
> comments investigate enabling additional Checkstyle violations to
> automate these suggestions?
>
> On Wed, Oct 08, 2014 at 06:53:47PM -0700, Adrian Cole wrote:
>> Hi, team.
>>
>> Having done sweeping change across a dozen providers I totally feel this
>> pain.
>>
>> In short, we know there are terrible malpractice like abstract builder for
>> one field in existing code, yet we don't do anything about it. Then, when
>> someone new comes in, somewhere between us telling them to delete extra
>> spaces, we ack that the thing we know sucks is something they pasted.
>>
>> It is reasonable for them to not know the right answer or to be overwhelmed
>> by what seems to be a timesoaking response to an innocent change.
>>
>> It isn't hard to fixup the existing providers so that they are already good
>> examples by the time someone goes to change them. I.e. they don't carry
>> behavior we know is awful. I tried to do some of that when I did the
>> unasyncing.
>>
>> This stuff happens, basically a norm of tech debt. Now that we have
>> checkstyle sorted, perhaps we can start paying down this sort of debt?
>>
>> I will volunteer to undebt one provider for each provider someone else
>> volunteers for. Anyone with me?
>>
>> -A
>> ---------- Forwarded message ----------
>> From: "inbar stolberg" <notificati...@github.com>
>> Date: Oct 8, 2014 4:51 PM
>> Subject: Re: [jclouds] cinder availability zones api + list call
>> implemented (#560)
>> To: "jclouds/jclouds" <jclo...@noreply.github.com>
>> Cc:
>>
>> In
>> apis/openstack-cinder/src/main/java/org/jclouds/openstack/cinder/v1/extensions/ExtensionNamespaces.java:
>>
>> > + * See the License for the specific language governing permissions and
>> > + * limitations under the License.
>> > + */
>> > +
>> > +package org.jclouds.openstack.cinder.v1.extensions;
>> > +
>> > +/**
>> > + * Extension namespaces
>> > + */
>> > +public final class ExtensionNamespaces {
>> > +
>> > + /**
>> > + * Admin Action extension
>> > + */
>> > + public static final String ADMIN_ACTIONS =
>> > "http://docs.openstack.org/ext/admin-actions/api/v1.1";;
>> > +
>>
>> 90% of this code is copy paste of existing code from the same branch not
>> old code either ...
>> I have no problem fixing this but doesnt seem like there is cosistency in
>> the code if i can copy paste some thing and get rejected over 100 lines....
>> :/
>>
>> —
>> Reply to this email directly or view it on GitHub
>> <https://github.com/jclouds/jclouds/pull/560/files#r18620246>.
>
> --
> Andrew Gaul
> http://gaul.org/
