[GitHub] tomcat issue #74: added javadoc comment
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/74 I saw the SVN commits -- thanks! --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user markt-asf commented on the issue: https://github.com/apache/tomcat/pull/74 I think I've pulled the obvious wins into Tomcat. Take a look and if you think there is merit in further changes please feel free to open another pull request. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/74 Sounds good. If you have more specific guidelines in mind then I'd be happy to implement/refactor accordingly. Please advise if so. Thanks. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user markt-asf commented on the issue: https://github.com/apache/tomcat/pull/74 There is definitely some value here although we might not choose to skip some parts of the patch. Let me pull in the obvious stuff and then we can see what is left. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/74 TBH upon further inspection the benefit of this patch is limited since most filters do extend FilterBase. While I do think that it provides cleaner code that can be helpful for future filters as well, I understand if you choose to reject it. Best, Igal --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/74 FYI: It's easier to see the changeset via this link (adding `?w=1` to a GitHub PR URL ignores same-line whitespace changes): https://github.com/apache/tomcat/pull/74/files?w=1 --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/74 I plan to refactor the rest of the Filters as I did for `CorsFilter`, but wanted you to see first what I meant. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/74 I believe that this will both reduce the overall volume of code and make the code more maintainable moving forward. I usually try to submit small patches so that they're easier to review and accept though, so the reduction of code might only be visible in the second step. I will work on it a bit and see how it goes. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user markt-asf commented on the issue: https://github.com/apache/tomcat/pull/74 Generally, I'm in favour of refactoring that reduces the overall volume of code. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Github user isapir commented on the issue: https://github.com/apache/tomcat/pull/74 @markt-asf - What do you think about changing `FilterBase` so that it extends `javax.servlet.GenericFilter`? That way we can use methods like `getInitParameter()` and have a single base class and "cleaner" code. For example, ATM some filters extend `org.apache.catalina.filters.FilterBase` (e.g. ExpiresFilter, FailedRequestFilter) and others extend `javax.servlet.GenericFilter` (e.g. CorsFilter, RemoteIpFilter). I can provide a patch if you agree. --- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
