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
>>
>>
>
