Skip to content

Conversation

@jbduncan
Copy link
Contributor

No description provided.

@jbduncan
Copy link
Contributor Author

jbduncan commented Mar 7, 2017

I do realise that this PR is 1. rather large and 2. that it came out of the blue, so I'd be happy to discuss the merits of it with someone from the google-java-format team, and even to break it down into smaller PRs if desired.

@jbduncan
Copy link
Contributor Author

jbduncan commented Apr 4, 2017

ping

@jbduncan
Copy link
Contributor Author

PR branch is now rebased on google/google-java-format/master.

@jbduncan
Copy link
Contributor Author

@cushon, I wonder if it would it be possible to start discussing this PR soon?

@cushon
Copy link
Collaborator

cushon commented Oct 12, 2017

It's not clear to me this is a readability improvement. The type argument can help document empty collections, e.g. Optional.<ExpressionTree>absent(), Optional.<TypeWithDims>absent() vs. Optional.absent(), Optional.absent().

@jbduncan
Copy link
Contributor Author

It's not clear to me this is a readability improvement. The type argument can help document empty collections, e.g. Optional.absent(), Optional.absent() vs. Optional.absent(), Optional.absent().

@cushon Ah yes, okay. I see what you mean.

Do you have any thoughts about the other changes I've made in this PR?

E.g.
Iterators.<ExpressionTree>peekingIterator(arguments.iterator());
-->
Iterators.peekingIterator(arguments.iterator());?

I'd totally understand if you'd rather that I just close this PR at this point. :)

@cushon
Copy link
Collaborator

cushon commented Oct 12, 2017

I think most of it looks good, but I may pick-and-choose a bit when I merge it.

For some of the ImmutableList.of() cases it might be better to use /* parameterName= */ comments than type arguments.

@cushon cushon closed this in 2ec27b5 Oct 14, 2017
@jbduncan jbduncan deleted the remove-redundant-type-args branch October 14, 2017 13:37
@jbduncan
Copy link
Contributor Author

LGTM @cushon. Thank you very much for merging in and adjusting my PR as was needed. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants