Menu
▾
▴
Re: [Geotools-devel] recent changes to property access on the stable branch?
|
From: Niels <nie...@cs...> - 2010-10-20 03:22:10
|
I rolled back on the branches... On 20/10/10 10:43, Justin Deoliveira wrote: > Sigh... indeed the patch was there. My apologies. When I looked at the > issue I thought that the 3 patches were all the same patch just > different versions of it. So I only looked at the top one with was > only geoserver app schema changes. Again sorry for that. > > About the changes in question, yes I did indeed look them over and was > in favor, but only on the trunk. Although it seems that was a mistake > and we are in agreement there. > > Anyways, apologies to all for making such a big fluff. I will try to > be more careful about reviewing patches in the future. > > -Justin > > On Tue, Oct 19, 2010 at 8:11 PM, Niels <nie...@cs...> wrote: > > Hi, > > Hmmm, I don't understand why you would think that the changes were > only to app-schema, the diff file has been attached to the issue > since 19 sept and was updated on the 28th of september, including > all the changes, just as you say it should be. Also, these changes > have been discussed extensively on the jira issue and on this > mailing list. From these discussions and your response on the jira > issue, everyone here really assumed that you approved the changes. > I don't really understand why you can't see the diff file, is that > possible? > > The update on the branch though is indeed a mistake from our end. > I guess this should be rolled back again. > It was indeed agreed the changes would only be applied to trunk, > so I take responsibility for that. My excuse is that this was my > first commit and I was given explicit instructions to also update > the branch, though my colleagues didn't realise that these > specific update wasn't supposed to go to the branch. > > Some communication errors, I guess! > > Niels > > > > On 20/10/10 08:16, Justin Deoliveira wrote: >> >> >> On Tue, Oct 19, 2010 at 6:07 PM, Jody Garnett >> <jod...@gm... <mailto:jod...@gm...>> wrote: >> >> I am not up to speed on the discussion; although I have been >> trying to catch up on Jira. >> >> I did expect some changes to XPathPropertyAccessor - but only >> on trunk as you indicated (this was the subject of the >> performance discussion prior to foss4g). Checking ... >> GEOT-3066 is correct. If you look at the first comment for >> that Jira it does indicate that the extent of the changes ... >> including XPathPropertyAccessorFactory. >> >> >> Ok, so perhaps I should have read the comments a little closer. >> But why was no diff posted or updated on the issue? Usually when >> a jira is sent to somebody for review a relevant/updated patch is >> attached to the issue, and the reviewer is not expected to go >> through all the comments trying to track down a diff? >> >> And about the change at hand. Yes it was discussed in detail on >> the mailing list. I brought it up because of my reservations >> about how it affected backwards compatabilty. And there it was >> decided on a approach that would keep things backward compatible. >> It was also mentioned that this should not be done on the stable >> branch, but perhaps that part should have been more clear. >> Quoting from email: >> >> "This breaks backward compatibility and i think requires more >> discussion. There is much more use of geotools beyond test cases >> and even beyond geoserver and udig. If there is one thing i >> learned at foss4g it is that there are lots of people using >> geotools, even though they don't take parts in discussions on the >> developer list. >> >> So i think this change requires more discussion and probably a >> proposal. And it certainly can't take place on stable branch imo." >> >> >> Jody >> >> On 20/10/2010, at 9:58 AM, Justin Deoliveira wrote: >> >>> I was reviewing the commit logs today and shocked to see >>> that recent changes revolving around property access and the >>> feature model are being committed to the stable branch? >>> >>> I am also quite confused by the commit logs. For instance, I >>> was recently asked to review a patch on >>> http://jira.codehaus.org/browse/GEOT-3066. I looked it over >>> and since it only contained files in the app-schema modules >>> i did not really have much to say. But then looking at the >>> commit that occurred: >>> >>> Author: NielsCharlier >>> Date: 2010-10-19 00:01:43 -0700 (Tue, 19 Oct 2010) >>> New Revision: 36280 >>> >>> Modified: >>> >>> branches/2.6.x/modules/extension/xsd/xsd-core/src/main/java/org/geotools/xml/XPathPropertyAccessorFactory.java >>> >>> branches/2.6.x/modules/extension/xsd/xsd-core/src/test/java/org/geotools/xml/XPathPropertyAcessorTest.java >>> >>> branches/2.6.x/modules/library/main/src/main/java/org/geotools/filter/AttributeExpressionImpl.java >>> >>> branches/2.6.x/modules/library/main/src/main/java/org/geotools/filter/FidFilterImpl.java >>> >>> branches/2.6.x/modules/library/main/src/main/java/org/geotools/filter/expression/PropertyAccessors.java >>> >>> branches/2.6.x/modules/unsupported/app-schema/app-schema/src/main/java/org/geotools/data/complex/config/AppSchemaDataAccessConfigurator.java >>> >>> branches/2.6.x/modules/unsupported/app-schema/app-schema/src/main/java/org/geotools/filter/expression/FeaturePropertyAccessorFactory.java >>> >>> branches/2.6.x/modules/unsupported/app-schema/app-schema/src/test/java/org/geotools/filter/expression/FeaturePropertyAccessorTest.java >>> >>> branches/2.6.x/modules/unsupported/app-schema/app-schema/src/test/resources/test-data/TimeSeriesTest_properties.xml >>> Log: >>> provides error reporting for invalid columns in app-schema >>> mapping file, see GEOT-3066 >>> >>> Ummmmm.... am I missing something here? >>> >>> Aside from the commits being way off from the jira issue I >>> thought it was discussed before in both jira and on the >>> developer list that this change was only going to be made on >>> the trunk/unstable branch and not on 2.0.x which is our >>> current stable branch. >>> >>> Is there something I am missing? >>> >>> -- >>> Justin Deoliveira >>> OpenGeo - http://opengeo.org <http://opengeo.org/> >>> Enterprise support for open source geospatial. >>> >>> ------------------------------------------------------------------------------ >>> Download new Adobe(R) Flash(R) Builder(TM) 4 >>> The new Adobe(R) Flex(R) 4 and Flash(R) Builder(TM) 4 (formerly >>> Flex(R) Builder(TM)) enable the development of rich >>> applications that run >>> across multiple browsers and platforms. Download your free >>> trials today! >>> http://p.sf.net/sfu/adobe-dev2dev_______________________________________________ >>> Geotools-devel mailing list >>> Geo...@li... >>> <mailto:Geo...@li...> >>> https://lists.sourceforge.net/lists/listinfo/geotools-devel >> >> >> >> >> -- >> Justin Deoliveira >> OpenGeo - http://opengeo.org >> Enterprise support for open source geospatial. >> > > > -- > *Niels Charlier* > > Software Engineer > CSIRO Earth Science and Resource Engineering > Phone: +61 8 6436 8914 > > Australian Resources Research Centre > 26 Dick Perry Avenue, Kensington WA 6151 > > > > > -- > Justin Deoliveira > OpenGeo - http://opengeo.org > Enterprise support for open source geospatial. > -- *Niels Charlier* Software Engineer CSIRO Earth Science and Resource Engineering Phone: +61 8 6436 8914 Australian Resources Research Centre 26 Dick Perry Avenue, Kensington WA 6151 |