You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by isapir <gi...@git.apache.org> on 2017/10/02 18:14:33 UTC
[GitHub] tomcat pull request #74: added javadoc comment
GitHub user isapir opened a pull request:
https://github.com/apache/tomcat/pull/74
added javadoc comment
Took me a few minutes to figure out what the method is doing (and how) so added a JavaDoc comment to clarify.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/isapir/tomcat update-01
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tomcat/pull/74.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 #74
----
commit 3f878e26b32d9ac0d309998fa6a3a30c76757ca9
Author: Igal Sapir <de...@21solutions.net>
Date: 2017-10-02T18:12:42Z
added javadoc comment
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat pull request #74: added javadoc comment
Posted by markt-asf <gi...@git.apache.org>.
Github user markt-asf closed the pull request at:
https://github.com/apache/tomcat/pull/74
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by isapir <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by isapir <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by markt-asf <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by isapir <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by markt-asf <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by isapir <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by isapir <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by isapir <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by isapir <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #74: added javadoc comment
Posted by markt-asf <gi...@git.apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org