On 06/10/2016 06:31 PM, Stanislav Laznicka wrote:
> On 06/08/2016 02:06 PM, Florence Blanc-Renaud wrote:
>> On 06/08/2016 10:07 AM, Petr Spacek wrote:
>>> On 7.6.2016 15:11, Stanislav Laznicka wrote:
>>>> Hello,
>>>> Thank you for your patch. As the thin-client patches were pushed in the
>>>> meantime, the patch won't apply. Could you please send a rebased version?
>>>> Also, I have a few comments to the patch:
>>>> 1) I think that the commit message should be rather a brief conclusion to 
>>>> the
>>>> changes made in the commit. This could help for faster orientation in the
>>>> changes that were made to a certain part of code should you be searching 
>>>> for a
>>>> bug introduced by a commit. Should some more info be required, it can be 
>>>> added
>>>> to the ticket. Could you therefore shorten the commit message?
>>> (My personal opinion, no golden standard.)
>>> Honestly I disagree with Standa. Yes, the commit message seems to be a bit
>>> long but *tickets* are not the best place to put *technical* information 
>>> into.
>>> Tickets are planning tool but keep in mind that Trac may/will vanish one day
>>> and all we will have will be (Git?) repo.
>>> I would recommend putting the comment about expected input format into code
>>> comments somewhere around batch command definition.
>>> This would reduce commit message (roughly, the text needs to be adapted) to
>>> part starting 'The code did not check the format of ' ... which is perfectly
>>> reasonable description of the change.
>>> IMHO.
>>> Petr^2 Spacek
>>>> 2) Please do not add the tickets to comments in the code. You can use git
>>>> blame -L or git log -L to see in which commits were the changes introduced 
>>>> to
>>>> a certain part of a file, these commits should include the ticket number if
>>>> more info is needed.
>>>> Standa
>>>> On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:
>>>>> Hi all,
>>>>> the following patch checks the format of parameters passed to a method
>>>>> called through the batch command. I picked the ConversionError for invalid
>>>>> parameters format but this choice can be discussed if you have better
>>>>> suggestions...
>>>>> Fixes:https://fedorahosted.org/freeipa/ticket/5810
>>>>> -- 
>>>>> Florence Blanc-Renaud
>>>>> Identity Management Team, Red Hat
>> Hi,
>> please find an updated patch version with a less verbose commit msg. I also 
>> removed the reference to ticket # in the code.
>> Flo.
> Hello,
> Thank you for your updated patch. I have just one small issue that maybe 
> exceeds 
> the scope of this ticket. If the check for dictionary instance in list:
> +            if not isinstance(arg, dict):
> +                raise errors.ConversionError(
> +                    name='methods',
> +                    error=_(u'must contain dict objects'))
> fails at a further member of the list, by raising an exception, you will lose 
> information about execution of all the previous commands but these were 
> already 
> executed. This hasn't seem to be an issue until now so I wonder if it is a 
> problem or not.

Right, this is something that we will need to address but not in scope
of this ticket and probably not 4.4 release.

> So far, batch commands have only been utilized in the WebUI so I am adding 
> Petr 
> for opinions on how to handle this properly so that WebUI could react to it 
> should it ever happen (although AFAIK this should never happen for batch 
> commands called from the UI).

Web UI doesn't care because it sends it correctly :) The bug is trying
to use batch command while talking directly to API - e.g. because of
performance reasons.

> Standa
Petr Vobornik

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to