On 14.06.2016 08:04, Stanislav Laznicka wrote:
On 06/13/2016 10:15 AM, Petr Vobornik wrote:
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:
Thank you for your patch. As the thin-client patches were pushed
meantime, the patch won't apply. Could you please send a rebased
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*
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
comments somewhere around batch command definition.
This would reduce commit message (roughly, the text needs to be
part starting 'The code did not check the format of ' ... which is
reasonable description of the change.
2) Please do not add the tickets to comments in the code. You can
blame -L or git log -L to see in which commits were the changes
a certain part of a file, these commits should include the ticket
more info is needed.
On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:
the following patch checks the format of parameters passed to a
called through the batch command. I picked the ConversionError
parameters format but this choice can be discussed if you have
Identity Management Team, Red Hat
please find an updated patch version with a less verbose commit
msg. I also
removed the reference to ticket # in the code.
Thank you for your updated patch. I have just one small issue that
the scope of this ticket. If the check for dictionary instance in list:
+ if not isinstance(arg, dict):
+ raise errors.ConversionError(
+ error=_(u'must contain dict objects'))
fails at a further member of the list, by raising an exception, you
information about execution of all the previous commands but these
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
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
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
Thank you for the explanation. ACK, then.
Pushed to master: 2c7ec27ad94a5a369c7d8a45dcef66a18479900b
Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code