commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Shaposhnikov <...@tut.by>
Subject Re: [configuration] AbstractConfiguration
Date Mon, 29 Sep 2003 23:07:31 GMT
Eric,

I perform code formatting of my changes to sources. See
attached patch file. I also use Eclipse for the development.
Unfortunately I am behind firewall, so I can't access cvs
from eclipse (because for some reason it refuses to work with
socks5 proxy server), so I create patch file using cvs diff
command. Then I try to apply this patch to the unmodified
sources using Eclipse and everything looks fine.

As for checkstyle I think that It report a lot of useless
warning. F.e. about final parameters. May be we can modify
checkstyle configuration to turn them off or just ignore
them. Now AbstractConfiguration has 175 warnings and
BaseConfiguration - 21.

I modify testBoolean to return Boolean instead of String.

As for additional tests it seems to me that existing tests
cover addProperty/addPropertyDirect functionality. Actually
this tests were very helpful for me when I perform
refactoring.

Also I think that later I can refactor other Configuration
classes to extend AbstractConfiguration.


On 17:28 Mon 29 Sep     , Eric Pugh wrote:
> [configuration] AbstractConfigurationKonstantin,
> 
> The changes look good.  And I checked through the Turbine codebase, and the
> changes you propose don't look like they will cause problems....
> 
> Could you do me a favor and submit a patch file?  Not sure what editor you
> use, but Eclipse does a good job of creating patch files.  Also, run the
> maven site and make sure your changes don't cause more checkstyle errors.
> You seem to use 2 spaces for a tab versus 4, a couple other little things as
> well.  Although actually the BaseConfiguration needs lots of checkstyle help
> as it has 194 violations!
> 
> Don't forget to add yourself to the contributor list in the project.xml and
> the @author tags;-)
> 
> I agree with the testBoolean, it should return a boolean value I think as
> well.
> 
> As far as the addProperty/addPropertyDirect, I would recommend that you add
> a unit test to the original code that tests it.  then, apply your changes,
> and verify the tests work the same way.  To be honest, I never had to look
> too closely at that part.
> 
> So, if you can try and resolve some of those questions, and get the
> formatting in line with the standard style then I'll be happy to commit
> these changes.  And then we can discuss your JDBC implementation which I am
> very curious to see...
> 
> Eric
>   -----Original Message-----
>   From: Konstantin Shaposhnikov [mailto:k8n@tut.by]
>   Sent: Sunday, September 28, 2003 8:16 PM
>   To: 'Jakarta Commons Developers List'
>   Subject: [configuration] AbstractConfiguration
> 
> 
>   Hello Eric,
> 
>   Pease see attached files:
>     AbstractConfiguration.java - AbstractConfiguration class,
>   actually this is slightly modified BaseConfiguration class
>   (I've  addes some methods and made some methods abstract)
>     BaseConfiguration.java - BaseConfiguration class modified
>   to extend Abstract configuration
>     TestBaseConfiguration.java - I add one test method to test
>   new behaviour of getString method and default configuration.
> 
>   All tests are passed succesfully (maven test :) .
> 
>   I also put some my comments in the source code. Please
>   review them.
> 
>   You are talking about getObject method but there is no such
>   method in Configuration interface. May be it will be usefull
>   to add it?
> 
>   As for a JDBCConfiguration class, I actually need very
>   specific to my project configuration implementation (using
>   Hibernate and special logic to access database). But we can
>   discuss this class and may be I'll implement them for
>   commons-configuration project.
> 
>   In any case I think that first we should refactor existing
>   Configuration implementations to use AbstractConfiguration
>   (of course if you accept it).
> 
>   On 17:41 Fri 26 Sep, Eric Pugh wrote:
>   > Konstantin,
>   >
>   > I like your idea about trying to make things easier.  I have been
> wanting to
>   > write a JDBCConfiguration class, but haven't gotten around to it.
>   >
>   > The BaseConfiguration works the way it does I think because it keeps
> config
>   > values in two seperate lists, correct?  One in memory, and one loaded by
> the
>   > user?
>   >
>   > What if we instead create an AbstractConfiguration class that overrides
>   > everything.  Then, when you implement your own, you just override the
>   > methods you want?  Similar to the approach taken by
> java.util.AbstractList?
>   >
>   > If you can supply the code with unit tests, I would be very happy to
> review
>   > and commit it.
>   >
>   > As far as the NoSuchElementException etc.. I actually think only
> getObject
>   > should return null..  Or none of them maybe..
>   >
>   > However, your idea for getProperty(String ke, Object defaultValue) would
> it
>   > return an Object?  How would that be different then getObject()?
>   >
>   > Also, if you want to donate your JDBCConfiguration, it would be much
>   > appreciated...
>   >
>   > Eric Pugh
>   >
> 
> 

Mime
View raw message