On Fri, May 8, 2009 at 5:10 PM, Justin Deoliveira <[email protected]>wrote:
> 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.
Ok, thank you very much then. I'll review your patches asap, in order to
restore the situation.
Daniele
>
>
> -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.
>
>
--
-------------------------------------------------------
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
-------------------------------------------------------
------------------------------------------------------------------------------
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
