No need to apologize. I am sympathetic to your deadline and I do not mean to come across as over protective of that code but when I turned restconfig from a community module to an extension, it required a lot of cleanup. I would like to ensure we maintain its current quality. A code review process is the best way to do that imo.
So about the current changes, I plan to rework your commits a bit, maintaining the functionality you added. I can post patches for you to review before I commit. -Justin Daniele Romagnoli wrote: > Hi Justin, > I'm really sorry about this. > Next time I'll follows the steps you are talking about. > I was working on the coverages ingestion and I needed to re-enable some > capabilities available in 1.7.2 for an urgent application. > I was too hasty. Sorry again. > > > On Fri, May 8, 2009 at 4:22 PM, Justin Deoliveira <[email protected] > <mailto:[email protected]>> wrote: > > Reviewing the commits more closely I definitely have a few concerns > which affirms my preference to have these changes reviewed before hand. > > * The method added to catalog builder seems unnecessary to me. I think > just the plain buildLayer can be called, and then properties overrideen > after the fact, instead put putting them into a map and passing it into > the method. It also does not sync up with any of the other methods in > that class. I would like to keep that class as consistent as possible. > > * You have made changes to api without any discussion. Supporting > wmspath and and style as query parameters is not documented as api > options: > > > http://gridlock.openplans.org/geoserver/1.7.x/doc/user/extensions/rest/rest-config-api.html#coverage-stores > > > The previous RESTConfig allowed to handle them. I had tought during > migration to extensions they have been forgotten. > > > > <http://gridlock.openplans.org/geoserver/1.7.x/doc/user/extensions/rest/rest-config-api.html#coverage-stores> > > Not saying we should not have them, but this is an extension now, not a > community module anymore, such things require discussion before hand. > > > You are Right. > > > > * Many of the changes seem a bit arbitrary, changing variable > declarations to final for instance. Again, for consistency sake I prefer > that you delegate to how the rest of the class / method does things when > making chagnes. > > * Setting the layer name as the entity is again an api change, and one I > don't agree with. It is more RESTful to set the location of the resource > using the appropriate HTTP header, 'Location' like we do for the POST > case. Also, the indication of where the resource should be down with a > full uri, just returning the name assumes that URI's are non-opaque, ie > the client has pre-knowledge of how URI's are built, which is another > REST nono. > > > Do you think it would be better if I revert my changes? > Daniele > > > > > > -Justin > > Justin Deoliveira wrote: > > Hi Daniele, > > > > I see you are making commits on 1.7.x to restconfig and main. I would > > appreciate posting patches for pre review first before committing. > > Especially in this case where we have not yet wrapped up discussion > > about how to handle the PUT issue from previous email. > > > > I am not trying to hamper work here but I have been deemed the > > maintainer of that extension, and while there are no hard rules > in place > > about who is allowed to commit, i would appreciate being to review > > commits before they go through. > > > > -Justin > > > > > -- > Justin Deoliveira > OpenGeo - http://opengeo.org > Enterprise support for open source geospatial. > > > ------------------------------------------------------------------------------ > The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your > production scanning environment may not be a perfect world - but > thanks to > Kodak, there's a perfect scanner to get the job done! With the NEW > KODAK i700 > Series Scanner you'll get full speed at 300 dpi even with all image > processing features enabled. http://p.sf.net/sfu/kodak-com > _______________________________________________ > Geoserver-devel mailing list > [email protected] > <mailto:[email protected]> > https://lists.sourceforge.net/lists/listinfo/geoserver-devel > > > > > -- > ------------------------------------------------------- > Eng. Daniele Romagnoli > Software Engineer > > GeoSolutions S.A.S. > Via Carignoni 51 > 55041 Camaiore (LU) > Italy > > phone: +39 0584983027 > fax: +39 0584983027 > mob: +39 328 0559267 > > > http://www.geo-solutions.it > > ------------------------------------------------------- > -- Justin Deoliveira OpenGeo - http://opengeo.org Enterprise support for open source geospatial. ------------------------------------------------------------------------------ The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your production scanning environment may not be a perfect world - but thanks to Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700 Series Scanner you'll get full speed at 300 dpi even with all image processing features enabled. http://p.sf.net/sfu/kodak-com _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
