[GitHub] tomcat pull request: Add JASPIC API (JSR 196) to build process
Github user fjodorver commented on the pull request: https://github.com/apache/tomcat/pull/21#issuecomment-110630412 2,4,5) I use ant validate for checkstyle validation. Is it enough? 3) It looks like we need to rewrite AuthConfigFactory anyway (for example, possible memory leaks, huge method and so on). I personally prefer to introduce small methods, which makes code reading much easier, because they work as self-commented code. Also, it simplifies code testing. As a bonus, in simple methods have shorter lines. For example, in current implementation need to introduce an ugly final variable, instead of just getting the correct value and make the first variable final. Second thing is guard clauses - usually I prefer to make such checks in the beginning of the method. It's quite good to get rid of necessary indentation and makes code lines shorter. 6. I've refined method order and some signatures (there are runtime exceptions declared, which is not necessary). However, constants in AuthConfigFactory are used for internal purposes, so I'd proposed security management as separate patch. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat pull request: Add JASPIC API (JSR 196) to build process
Github user fjodorver commented on the pull request: https://github.com/apache/tomcat/pull/21#issuecomment-110251900 It's updated pull request. 1) Build.xml configuration in separate commit now 2) Fixed imports and formatting 3) Method refactoring has been made separately. Geronimo uses OSGi stuff to create this factory class, I see you had already fixed that. Threre are two reasons why this refactoring required in my opinion: get rid of extra final variable and get rid of extra indent. 4) removed version from javadoc 5) fixed checkstyle errors 6) What about JASPIC 1.1 changes in API, I don't see any changes between this version. It looks like 1.1 changes is more like internal ones, and API classes are still the same. Correct me if I am wrong. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat pull request: Add JASPIC API (JSR 196) to build process
GitHub user fjodorver opened a pull request: https://github.com/apache/tomcat/pull/21 Add JASPIC API (JSR 196) to build process I'd added jaspic api support in this commit. You can merge this pull request into a Git repository by running: $ git pull https://github.com/fjodorver/tomcat feature/jaspic-api Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tomcat/pull/21.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #21 commit c0274264ebc5b434e1446ea032302fd6df2a7893 Author: Fjodor Vershinin fjo...@vershinin.net Date: 2015-06-07T20:09:43Z Add JASPIC API (JSR 196) to build process --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
