[Freeipa-devel] Patch for the INI parser. Ceanup. Prep for INI validation.

[Dmitri Pal]

Simo Sorce wrote:
> On Thu, 2009-04-16 at 16:50 -0400, Stephen Gallagher wrote:
>   
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Stephen Gallagher wrote:
>>     
>>> Dmitri Pal wrote:
>>>       
>>>> Hi,
>>>>         
>>>>    INI parser. Cleanup. Prep for INI validation.
>>>>      This patch addresses several issues:
>>>>    a) Cleaning unit test to match coding standard
>>>>    b) Replace tabs with spaces - I do not know where they came
>>>>    but there were some.
>>>>    c) Allowing to read file and keep aside a collection
>>>>    of K-V pairs where key is the key in the INI file and value is the
>>>>    line number on which line the key appears.
>>>>    d) There will be different kinds of errors so
>>>>    error printing function was abstracted.
>>>>    g) Placeholders for other printing functions have been introduced.
>>>>         
>>>
>>>       
>>>> ------------------------------------------------------------------------
>>>>         
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>         
>>> Nack
>>>
>>> Do not include so many different changes in a single patch. Of note,
>>> never include formatting changes alongside functional changes. This
>>> needs to be broken up into at least two patches, one for the formatting
>>> and the other for the functional changes.
>>>
>>> There is no need to internationalize the empty string. It's a waste of
>>> gettext macros.
>>>
>>> I am morally opposed to the use of inline in a public API, but I can
>>> live with it.
>>>
>>>
>>>
>>>       
>> After several off-list discussions, I'm going to push this patch, and
>> Dmitri will follow up with some additional fixes.
>>
>> Highlights: his copy of git is broken, so breaking the patch in two is
>> non-trivial. The strings are placeholders, so it's fine to
>> internationalize them. Inline may be removed in a subsequent patch.
>>
>> Ack and pushed.
>>     
>
> Please next time make these discussions public.
> There is no pressing need to push this patch immediately, if Dmitri has
> problems with his git tree, he better get it fixed before going forward.
> It's not an excuse to get poor patches in.
>
> Simo.
>
>   
Simo, the point is that:

a) There is no clear guidance about the "inline" thing. So it is not a 
bug and not an issue.
But I am willing to correct it anyways with the follow up patch.
b) The strings are placeholders and will be replaced by the full 
functionality and I am working on it.
The functions that are committed that have there "strings" are not yet 
used by any code.
Plus the code makes sure that these lines are never returned (the valid 
range is empty).
It just seemed inefficient to clear this code.
c) The only real issue is that formatting changes and real fixes are in 
the same patch. I would do my best to submit it as different patches in 
future.
It seems really inefficient to force me to redo it now especially when I 
have issues with git merge.

Simo I agree that should probably find the solution to the git problem 
but I currently do not see a way to do it without wasting a lot of time.
I am already behind on other things and with all this volume of the 
tasks I can't afford redoing things twice.
All the notes, comments and suggestions you and Steven give are factored 
and I do not repeat the mistakes twice.
So there is a bit of difference between a "poor" patch  and "sufficient 
under current circumstances patch".

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/