[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-15 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-521730229
 
 
   Also it is a pain to handle multiple log formats which also make formatting 
harder. I would propose a best effort(i.e. no rule to enforce this) on the fly 
reformatting on log style(string concat style) that when the line is too long 
and variable interception matters, use the pattern below.
   
   ```
   log.info("...{} ...{}", arg1, arg2)
   ```
   
   instead of string concat. This also works on `String.format`.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-15 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-521730229
 
 
   Also it is a pain to handle multiple log formats which also make formatting 
harder. I would propose a best effort on the fly reformatting on log 
style(string concat style) that when the line is too long and variable 
interception matters, use the pattern below.
   
   ```
   log.info("...{} ...{}", arg1, arg2)
   ```
   
   instead of string concat. This also works on `String.format`.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520634570
 
 
   With read comments above, I'd like to propose group rules in different 
concerns into different issue/pull request. Initially on the mailing list we 
reach a consensus on enabling checkstyle configuration per package/module 
instead of per rules but as described above different type of rules need 
different attention. Basically, it would be
   
   A basic checkstyle rules, that is, the same as current 
`checkstyle-strict.xml` excludes *Names rules, JavaDoc rules and LineBreak. 
Details as below.
   
   - A new JIRA issue to track a revisit of *Names rules as well as variable 
visibility. The leverage is between backward compatibility to strict 
visibility. For example `public static final` should be ALL_CAPS ideally but 
for backward compatibility we should retain; while a series of `(default) 
static final` is occasionally package-private but semantically should be 
`private static final`.
   
   - JavaDoc is not just about checkstyle. As filed in ZOOKEEPER-3469 we need a 
dedicated pass for reviewing changes in JavaDoc regard the changes in 
documentation scope as well.
   
   - LineBreak is suppressed as well. There is not a general ideal how to break 
a line and it requires a discussion on code style in Flink community[1]. I 
would prefer suppress it at least for now or even we reach a consensus that we 
don't need it.
   
   - Fair enough, log format deserves a JIRA issue although it might out of the 
scope here.
   
   And consequently, the basic checkstyle configuration focuses on whitespaces, 
imports, operators on a new line, redundant or out-of-order modifiers, TODO 
comments, and if/else also with curly.
   
   It's ok to me that this pull request as a prototype on bringing checkstyle 
to zookeeper-server and a total rebase require one or two days doesn't inflict 
too much. It is more important to see where our community decide to go.
   
   [1] 
https://lists.apache.org/thread.html/609112aa8d0a0d3260bfca4f34ca405f39e2f44bf6bcba010068266e@%3Cdev.flink.apache.org%3E


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520634570
 
 
   With read comments above, I'd like to propose group rules in different 
concerns into different issue/pull request. Initially on the mailing list we 
reach a consensus on enabling checkstyle configuration per package/module 
instead of per rules but as described above different type of rules need 
different attention. Basically, it would be
   
   A basic checkstyle rules, that is, the same as current 
`checkstyle-strict.xml` excludes *Names rules, JavaDoc rules and LineBreak. 
Details as below.
   
   - A new JIRA issue to track a revisit of *Names rules as well as variable 
visibility. The leverage is between backward compatibility to strict 
visibility. For example `public static` should be ALL_CAPS ideally but for 
backward compatibility we should retain; while a series of `(default) static` 
is occasionally package-private but semantically should be `private static`.
   
   - JavaDoc is not just about checkstyle. As filed in ZOOKEEPER-3469 we need a 
dedicated pass for reviewing changes in JavaDoc regard the changes in 
documentation scope as well.
   
   - LineBreak is suppressed as well. There is not a general ideal how to break 
a line and it requires a discussion on code style in Flink community[1]. I 
would prefer suppress it at least for now or even we reach a consensus that we 
don't need it.
   
   - Fair enough, log format deserves a JIRA issue although it might out of the 
scope here.
   
   And consequently, the basic checkstyle configuration focuses on whitespaces, 
imports, operators on a new line, redundant or out-of-order modifiers, and 
if/else also with curly.
   
   It's ok to me that this pull request as a prototype on bringing checkstyle 
to zookeeper-server and a total rebase require one or two days doesn't inflict 
too much. It is more important to see where our community decide to go.
   
   [1] 
https://lists.apache.org/thread.html/609112aa8d0a0d3260bfca4f34ca405f39e2f44bf6bcba010068266e@%3Cdev.flink.apache.org%3E


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520634570
 
 
   With read comments above, I'd like to propose group rules in different 
concerns into different issue/pull request. Initially on the mailing list we 
reach a consensus on enabling checkstyle configuration per package/module 
instead of per rules but as described above different type of rules need 
different attention. Basically, it would be
   
   A basic checkstyle rules, that is, the same as current 
`checkstyle-strict.xml` excludes *Names rules, JavaDoc rules and LineBreak. 
Details as below.
   
   - A new JIRA issue to track a revisit of *Names rules as well as variable 
visibility. The leverage is between backward compatibility to strict 
visibility. For example `public static` should be ALL_CAPS ideally but for 
backward compatibility we should retain; while a series of `(default) static` 
is occasionally package-private but semantically should be `private static`.
   
   - JavaDoc is not just about checkstyle. As filed in ZOOKEEPER-3469 we need a 
dedicated pass for reviewing changes in JavaDoc regard the changes in 
documentation scope as well.
   
   - LineBreak is suppressed as well. There is not a general ideal how to break 
a line and it requires a discussion on code style in Flink community[1]. I 
would prefer suppress it at least for now or even we reach a consensus that we 
don't need it.
   
   - Fair enough, log format deserves a JIRA issue although it might out of the 
scope here.
   
   And consequently, the basic checkstyle configuration focuses on whitespaces, 
imports, operators on a new line, redundant or out-of-order modifiers, TODO 
comments, and if/else also with curly.
   
   It's ok to me that this pull request as a prototype on bringing checkstyle 
to zookeeper-server and a total rebase require one or two days doesn't inflict 
too much. It is more important to see where our community decide to go.
   
   [1] 
https://lists.apache.org/thread.html/609112aa8d0a0d3260bfca4f34ca405f39e2f44bf6bcba010068266e@%3Cdev.flink.apache.org%3E


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520605920
 
 
   @hanm 
   
   >There are lots of changes to exception types thrown by functions, where it 
changes concrete exception types that's a function could actually throw to 
Exception (see some detailed examples in comments). What's the reason behind 
this change?
   
   Those changes are made limited on test* methods where otherwise a long line 
to be breakdown. A test* method should be always ok to be written thrown 
`Exception` because any verification is done in the method and no one should 
rely on a test* method. It itself is a top level method.
   
   
   I support your advice that
   
   > Pure formatting changes: white spaces, curly braces, indentations, etc.
   > None functional changes: remove unneeded imports, unused exceptions types, 
unneeded type parameters, etc.
   
   Let me see what I can do to rebase this pull request. Formerly I give every 
file an auto formatting phase, a glance phase and a fixing error phase. Maybe 
the middle phase is problematic. I should have do it by only a fixing error 
phase.
   
   By the way, following this suggestion we could suppress *NameCheck in this 
pass and give a dedicated pass to revisit it. That is, checkstyle rules 
restrict the pattern of method names, field names and variable names.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520605920
 
 
   @hanm 
   
   >There are lots of changes to exception types thrown by functions, where it 
changes concrete exception types that's a function could actually throw to 
Exception (see some detailed examples in comments). What's the reason behind 
this change?
   
   Those changes are made limited on test* methods where otherwise a long line 
to be breakdown. A test* method should be always ok to be written thrown 
`Exception` because any verification is done in the method and no one should 
rely on a test* method. It itself is a top level method.
   
   Apart from that, I support your advice that
   
   > Pure formatting changes: white spaces, curly braces, indentations, etc.
   > None functional changes: remove unneeded imports, unused exceptions types, 
unneeded type parameters, etc.
   
   Let me see what I can do to rebase this pull request. Formerly I give every 
file an auto formatting phase, a glance phase and a fixing error phase. Maybe 
the middle phase is problematic. I should have do it by only a fixing error 
phase.
   
   By the way, following this suggestion we could suppress *NameCheck in this 
pass and give a dedicated pass to revisit it. That is, checkstyle rules 
restrict the pattern of method names, field names and variable names.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520376969
 
 
   @enixon 
   
   > Do we know whether the change in visibility to some of the methods will 
effect curator or other third parties?
   
   There is no refactoring and nothing public changed. Only reformatting and 
some dead code deletion included. It is expected be unaware of any other third 
parties.
   
   > A question for others: with regards to the TODOs in the code, do we have 
any expectation that they are backed by JIRA tickets and, if so, how can them 
be linked to the ticket number?
   
   Nice question. We could have another pass(whether or not coupled within 
this) to investigate all TODOs and linked to the ticket and when there is no 
one, create one or delete TODO comment.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-11 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520221001
 
 
   saved with the last fixup. If we want to merge this pr into master in multi 
commits, ping me to rebase the commits or manually rebase and merge it.(merge 
the last fixup to the corresponding commit)


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-11 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520221001
 
 
   saved with the last fixup.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services