cshannon commented on PR #3064:
URL: https://github.com/apache/accumulo/pull/3064#issuecomment-1304547589
@ctubbsii - You are right that it may not be a complete set. In fact the
Google eclipse XML I found is almost 5 years old at this point so it may not
have everything.
As you pointed out, we probably don't diverge too much but there are a few
things that are different that I will point out below that would be nice to
change. I had figured starting with the Google style guide as a base would be
easiest but now I'm thinking maybe not since it's out of date. With your update
to the latest format in #3066 I'm thinking it's actually easiest to just tweak
that file with things we want to change.
I have used Intellij the last couple of years but I used to use Eclipse for
a long time so it wouldn't be too hard for me to download Eclipse again to make
changes and work with you to do it (or if you wanted to do it that's fine,
either way).
I went through the diff briefly and noticed the following main things.
There's probably more changes but these seemed to be the main ones that took up
most of the diff.
1. The number one thing of course is always requiring braces on control
statements. I think this is hugely important not just for readability but for
error prevention. It's easy to make mistakes inadvertently in the code if you
don't require braces for single line control statements. Part of this is
actually an errorpone change to require braces. The formatter will format the
braces if they are there but doesn't add it. I had to actually use Intellij to
add these and then turn on errorprone to fail if they are missing.
2. Spacing after a comma with Generics. Our current format looks like
`Iterator<Entry<Key,Value>> iterator();` and the new format looks like
`Iterator<Entry<Key, Value>> iterator();` which I prefer.
3. Control statement alignment, which we could change but I'm ok either way.
A couple of examples:
```
//Our current format
if (tLists.unassigned.size() + unloaded
> Manager.MAX_TSERVER_WORK_CHUNK * currentTServers.size()) {
//Updated Google format
if (tLists.unassigned.size() + unloaded > Manager.MAX_TSERVER_WORK_CHUNK
* currentTServers.size()) {
```
```
//Our current format
if (manager.getSteadyTime() - tls.suspend.suspensionTime
< tableConf.getTimeInMillis(Property.TABLE_SUSPEND_DURATION)) {
//Updated Google format
if (manager.getSteadyTime() - tls.suspend.suspensionTime < tableConf
.getTimeInMillis(Property.TABLE_SUSPEND_DURATION)) {
```
4. Some other line length changes.
```
//Current format
public TabletServers getParticipatingTabletServers(@PathParam("tableId")
@NotNull @Pattern(
regexp = ALPHA_NUM_REGEX_TABLE_ID) String tableIdStr) {
//Google format
public TabletServers getParticipatingTabletServers(@PathParam("tableId")
@NotNull
@Pattern(regexp = ALPHA_NUM_REGEX_TABLE_ID) String tableIdStr) {
```
5. Javadoc changes to move variable comments to the same line
```
//Current format
* @param key
* Same restrictions on key as {@link #append(Key, Value)}.
* @param value
* this parameter will be UTF-8 encoded. Must be non-null.
//Google format
* @param key Same restrictions on key as {@link #append(Key, Value)}.
* @param value this parameter will be UTF-8 encoded. Must be non-null.
```
Assuming we want to just start with our file as the basis (which I think
makes sense now as stated, the main thing would be going through and figuring
out what to modify to match. If you want I can grab Eclipse and try and tweak
our current file to make (at least some of) the changes so that it's easier to
compare instead of starting with the Google one. I can start from the new
commit added in main in #3066 and then just redo everything and force push up.
What do you 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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
