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.
+1 The commit message is very good and honestly I'd like to see more of such commit messages. > > 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. Batch command deserves a doc string, so that it would be visible in API browser but that is not subject of this ticket. Btw, expected format is already in the code. > > 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. +1 >> >> 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 >>> >>> >> >> >> >> > > -- Petr Vobornik -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
