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:[EMAIL PROTECTED] 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:[EMAIL PROTECTED] > 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 > > > >
