i probably spoke to hastily, let me get back to you later once i spend some
more time digging through the changes :)
On Sun, Nov 23, 2008 at 3:17 PM, Chris Chabot <[EMAIL PROTECTED]> wrote:
> ps one thing that (now:P) stands out is the subtle change to the output
> format detection. it used to be that:
>
> It first checked for a {post,get}['format'], and if those weren't set it
> checked the $_SERVER['CONTENT_TYPE'] and used that (going by the logic that
> if you stuff ATOM in, you would probably want ATOM out too)
>
> However in this patch it seems the output is now entirely based on the
> format query param, and no longer checks the content type?
>
>
> On Sun, Nov 23, 2008 at 3:13 PM, Chris Chabot <[EMAIL PROTECTED]> wrote:
>
>> That makes it a whole lot more readable and digestible, thanks!
>>
>>
>>
>>
>> On Sun, Nov 23, 2008 at 3:04 PM, Erik Gomersbach <[EMAIL PROTECTED]>wrote:
>>
>>> Hi Chris,
>>>
>>> I've split the patch into 6 smaller patches and rebased on the current
>>> trunk.
>>>
>>> Please let me know if this works for you!
>>>
>>> Cheers,
>>> Erik
>>>
>>> ----- Original Message ----- From: "Chris Chabot" <[EMAIL PROTECTED]>
>>> To: <[email protected]>
>>> Sent: Sunday, November 23, 2008 12:43 PM
>>>
>>> Subject: Re: [jira] Commented: (SHINDIG-705) Allow extending with new
>>> services without having to change ApiServlet code
>>>
>>>
>>> agreed
>>>>
>>>> and please make sure they apply cleanly on the current trunk :)
>>>>
>>>> On Sun, Nov 23, 2008 at 12:31 PM, Erik Gomersbach <[EMAIL PROTECTED]>
>>>> wrote:
>>>>
>>>> Ok, I'll split the one big patch into several smaller, more readable,
>>>>> patches. These patches won't be independent though, you agree?
>>>>>
>>>>> I did update the unit tests, that's right.
>>>>>
>>>>> Erik
>>>>>
>>>>> ----- Original Message ----- From: "Chris Chabot" <[EMAIL PROTECTED]>
>>>>> To: <[email protected]>
>>>>> Sent: Sunday, November 23, 2008 12:22 PM
>>>>> Subject: Re: [jira] Commented: (SHINDIG-705) Allow extending with new
>>>>> services without having to change ApiServlet code
>>>>>
>>>>>
>>>>>
>>>>> if you could re-base the diff from the latest trunk that should
>>>>> already
>>>>>
>>>>>> save
>>>>>> us a lot of -- and ++ on the formatting; I always prefer to have
>>>>>> patches
>>>>>> in
>>>>>> the right code format, otherwise they quickly become unreadable, as we
>>>>>> had
>>>>>> now :)
>>>>>>
>>>>>> maybe if you could also split up the patches into a rest/api/data
>>>>>> service
>>>>>> patch, and a --/++ input/output converters, that should make that a
>>>>>> lot
>>>>>> easier to grok too.
>>>>>>
>>>>>> Sorry for being such a pain in the backside, but with a total of > 150
>>>>>> mil
>>>>>> active users reached with this code, i tend to be
>>>>>> rather-safe-then-sorry
>>>>>> :)
>>>>>>
>>>>>> (ps i saw you did adjust the unit tests, right? having those in
>>>>>> correctly
>>>>>> is
>>>>>> important)
>>>>>>
>>>>>> On Sun, Nov 23, 2008 at 12:11 PM, Erik Gomersbach <[EMAIL PROTECTED]>
>>>>>> wrote:
>>>>>>
>>>>>> Hi Chris,
>>>>>>
>>>>>>>
>>>>>>> thanks for having a look at this.
>>>>>>>
>>>>>>> I agree that the patch seems to be much larger than expected, but I'm
>>>>>>> not
>>>>>>> sure how easy it will be to reduce the size, but maybe you have some
>>>>>>> idea
>>>>>>> on
>>>>>>> this.
>>>>>>>
>>>>>>> This is what the patch contains:
>>>>>>>
>>>>>>> The main changes are in the following files:
>>>>>>>
>>>>>>> RestRequestItem
>>>>>>> ApiServlet
>>>>>>> DataServiceServlet
>>>>>>>
>>>>>>> The other changes are mainly rearranging of existing code:
>>>>>>>
>>>>>>> The data format converters:
>>>>>>> -InputAtomConverter
>>>>>>> -InputJsonConverter
>>>>>>> -InputXmlConverter
>>>>>>> -Output AtomConverter
>>>>>>> -OutputJsonConverter
>>>>>>> -OutputXmlConverter
>>>>>>>
>>>>>>> I have replaced with data content converters:
>>>>>>> +InputActivityConverter
>>>>>>> +InputAppDataConverter
>>>>>>> +InputMessagesConverter
>>>>>>> +InputPersonConverter
>>>>>>> +OutputStandardConverter
>>>>>>>
>>>>>>> Especially this causes lots of +++ and --- in the patch.
>>>>>>>
>>>>>>> Same holds for the updates tests.
>>>>>>>
>>>>>>> Please let me know how I can improve this. I am using two spaces for
>>>>>>> indentation, so that should be in line with your latests commits ;-)
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Erik
>>>>>>>
>>>>>>> ----- Original Message ----- From: "Chris Chabot (JIRA)" <
>>>>>>> [EMAIL PROTECTED]>
>>>>>>> To: <[EMAIL PROTECTED]>
>>>>>>> Sent: Sunday, November 23, 2008 11:25 AM
>>>>>>> Subject: [jira] Commented: (SHINDIG-705) Allow extending with new
>>>>>>> services
>>>>>>> without having to change ApiServlet code
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> https://issues.apache.org/jira/browse/SHINDIG-705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12650011#action_12650011
>>>>>>>> ]
>>>>>>>>
>>>>>>>> Chris Chabot commented on SHINDIG-705:
>>>>>>>> --------------------------------------
>>>>>>>>
>>>>>>>> Hey Erik,
>>>>>>>>
>>>>>>>> First of all thanks for working on this!
>>>>>>>>
>>>>>>>> While trying to look through the patch, I couldn't help but noticing
>>>>>>>> it
>>>>>>>> --'s and ++'s a lot more then I would expect, a total of a 130k of
>>>>>>>> changes
>>>>>>>> is slightly more then expected :) ) (It would seem a lot of it is
>>>>>>>> just
>>>>>>>> code
>>>>>>>> formatting stuff)
>>>>>>>>
>>>>>>>> To be able to actually see what this does (and not make the
>>>>>>>> project's
>>>>>>>> code
>>>>>>>> formatting inconsistent) I would really love to have a bit more
>>>>>>>> concise
>>>>>>>> patch that only consists of the actual changes
>>>>>>>>
>>>>>>>> Allow extending with new services without having to change
>>>>>>>> ApiServlet
>>>>>>>>
>>>>>>>> code
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --------------------------------------------------------------------------
>>>>>>>>>
>>>>>>>>> Key: SHINDIG-705
>>>>>>>>> URL:
>>>>>>>>> https://issues.apache.org/jira/browse/SHINDIG-705
>>>>>>>>> Project: Shindig
>>>>>>>>> Issue Type: Improvement
>>>>>>>>> Components: RESTful API (PHP)
>>>>>>>>> Reporter: Erik Gomersbach
>>>>>>>>> Priority: Minor
>>>>>>>>> Attachments: patch2.txt
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Currently one is required to change the ApiServlet code to be able
>>>>>>>>> the
>>>>>>>>> extend Shindig with new services. This patch makes the extending of
>>>>>>>>> Shindig
>>>>>>>>> with new services configurable.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>> This message is automatically generated by JIRA.
>>>>>>>> -
>>>>>>>> You can reply to this email to add a comment to the issue online.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --------------------------------------------------------------------------------
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> No virus found in this incoming message.
>>>>>>> Checked by AVG - http://www.avg.com
>>>>>>> Version: 8.0.175 / Virus Database: 270.9.9/1805 - Release Date:
>>>>>>> 11/22/2008
>>>>>>> 10:34 AM
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>> No virus found in this incoming message.
>>>>> Checked by AVG - http://www.avg.com
>>>>> Version: 8.0.175 / Virus Database: 270.9.9/1805 - Release Date:
>>>>> 11/22/2008
>>>>> 10:34 AM
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --------------------------------------------------------------------------------
>>>
>>>
>>>
>>> No virus found in this incoming message.
>>> Checked by AVG - http://www.avg.com
>>> Version: 8.0.175 / Virus Database: 270.9.9/1805 - Release Date:
>>> 11/22/2008 10:34 AM
>>>
>>>
>>
>
