[prev in list] [next in list] [prev in thread] [next in thread]
List: jakarta-commons-dev
Subject: RE: [configuration] AbstractConfiguration
From: "Eric Pugh" <epugh () upstate ! com>
Date: 2003-09-30 15:56:49
[Download RAW message or body]
Re: [configuration] AbstractConfigurationI agree with the checkstyle errors
being kinda bogus on some of them.. I agree with you on the unit tests..
they may be tests, but they are also great learning tools!
I am patched C-C and it went in nice and clean, thanks.. I am going to test
it with some of my apps for a day and then commit. I want to make sure the
API change doesn't cause any problems.
Then I would love to see any other refactorings you have. Maybe once we
refactor to use AbstractCollection then would be time to push for a move out
of sandbox...
Eric Pugh
-----Original Message-----
From: Konstantin Shaposhnikov [mailto:k8n@tut.by]
Sent: Tuesday, September 30, 2003 1:08 AM
To: 'Jakarta Commons Developers List'
Subject: Re: [configuration] AbstractConfiguration
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
> >
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic