[GitHub] [zookeeper] TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-522197888 Thanks for your review @hanm ! @eolivelli I also gave it a second pass and reverted/addressed ugly formats by hand(line breaks almost, as mentioned above, hard to give a rule but will still digging). If you can give it another pass we possibly merge it before any conflict. 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-521730229
Also there is a pain multiple log format 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-521729375 Line breaks, i.e., confusing line formats request changed above, suffer from lack usage of local variables, which causes quite a lot long line without semantic awareness. I would like to add some local variables to help formatting. Since it is limited in local variables, it would not conflict outside world. 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-521273094 I have redone the task enable checkstyle configuration and resolve conflict. Basically it focuses on whitespaces, imports, operators on a new line, redundant or out-of-order modifiers, TODO comments, and if/else also with curly. For javadoc discussion, let's move to ZOOKEEPER-3469. For variable/class names discussion(whether change it or not on conflict with suggested name pattern), let's move to ZOOKEEPER-3507. For a proper line break strategy, or even we don't want it, let's discuss in ZOOKEEPER-3508. Log format issue is now tracked by ZOOKEEPER-3509. 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-521270729 retest this please 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520765229 For keeping comments, I can proceed the work on this pull request. But it is unreasonable I send a fixup onto current commits since the proposal(impl.) diverged quite a bit. I would redo based on master and force push to this branch if we want to continue on this thread. 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520701237 @eolivelli > All of the commits will be squashed into a single one. I am aware of this. In consideration of reviews, it would be better to start a new pull request? 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented 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, 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented 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. Besides 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520376969 > 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520330217 @nkalmar Thanks for your review and Thanks for @enixon 's comments. I would try to give a look and address them hours later. 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented 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. 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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520220611 Fails on ``` [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1559:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1634:8: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1644:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1825:85: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1882:80: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2086:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2194:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2420:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2484:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2548:8: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2555:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2629:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2755:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3009:8: Unable to get class information for @throws tag 'KeeperException.NoWatcherException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3023:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3057:8: Unable to get class information for @throws tag 'KeeperException.NoWatcherException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3066:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java:54:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java:128:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod] Audit done. [INFO] There are 20 errors reported by Checkstyle 8.17 with checkstyle-strict.xml ruleset. [ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1550,8] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'. [ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1559,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'. [ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1634,8] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'. [ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1644,14] (javadoc) JavadocMethod: Unable to get
[GitHub] [zookeeper] TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520219117 Done on all packages under zookeeper-server. Please review if you have spare time ^_^ 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
