Jan Luehe wrote:
> Hi Wolfram,
>
> thanks for making the changes!
>
> Your patch looks just perfect!
>
> I was able to apply your diffs just fine.
>
> I have sent the DOL related changes to the deployment lead (Hong
> Zhang), so she gets a chance to review them also before I commit them
> on your behalf.
>
> Some very minor things I noticed that you might want to keep in mind
> for any future patch submissions: :)
>
> - Use spaces instead of TABs
Hi Jan,
well I agree and usually take care about this. I have set the "Expand
Tabs to Spaces" in Netbeans. It still might happen when I use gedit or
vi. And I must admit that the tests I attached to the issue 7177 contain
a lot of tabs because of my heavy usage of gedit to edit these classes.
Yikes! I will pay more attention to this in the future.
>
> - Avoid lines longer than 80 characters (see Java Coding Conventions)
The 80 char margin is also highlighted in Netbeans - though I must admit
I didn't pay much attention to this. Personally I consider this to be
not very relevant nowadays given the widescreen monitors and what not.
But obviously I am wrong. So I will also care about that from now on ;-)
>
> I went ahead and made these "cosmetic" adjustments in the very few
> places in your diffs where they were needed.
>
> Also, I added a "return" statement here (in WebModule.java):
>
> + if (!(valve instanceof GlassFishValve) &&
> + !(valve instanceof Valve)) {
> + logger.log(Level.WARNING,
> + "Object of type classname " + className +
> + " not an instance of Valve or GlassFishValve");
> + }
>
Oops. That's right. A return would have been nice.
> I also went ahead and added a message key for the above warning
> message, so it can be localized (I made the same change in the other
> place in
> the existing code that had the same issue).
I have actually thought about this, but decided not to change any of the
existing code that was not directly related to the changed valve
behaviour. On the other hand: Cleaning up code is never a bad decision.
So probably I should have done that.
Thanks for your comments and corrections.
>
> That's it!
>
> Once I hear back from Hong, I will commit your changes on your behalf.
>
Given the other mails this has obviously been done in the meantime. This
actually feels quite good :-)
> You mentioned you also have a test case for this feature. Feel free to
> send it my way, so I can integrate it as well.
>
> Once your changes have been committed, we'll work with June Parks
> (cc'ed), so she can update the Developer Guide and also take care of
> deprecating the property based approach.
>
> Thanks again for your first patch and contribution, Wolfram!
> Excellent work!!
>
> Jan
More on this in an answer to the committer mail later on.
Thanks,
--
Wolfram