On Wed, Mar 02, 2016 at 01:43:03PM +0100, Petr Cech wrote: > On 03/02/2016 01:10 PM, Lukas Slebodnik wrote: > >On (02/03/16 13:02), Pavel Reichl wrote: > >>On 03/02/2016 12:53 PM, Lukas Slebodnik wrote: > >>>On (02/03/16 12:48), Pavel Březina wrote: > >>>>On 03/01/2016 03:54 PM, Pavel Reichl wrote: > >>>>>I added one more similar patch. > >>>>> > >>>>>sss_idmap_calculate_range can accept domain SID or range identifier on > >>>>>its input. Previous parameter name was misleading. > >>>> > >>>>Ack to both. They can sure be squashed before pushing but I don't care. > >>>I miss a link here. > >>> > >>>Anyway I would like to see a Sumit opinin about renaming variables. > >>>Because name of variables is one of hard things problems in IT > >>>http://martinfowler.com/bliki/TwoHardThings.html > >> > >>Might we could do an all hands meeting, > >>because we really don't want to underestimate such important change. > >> > >If the patch is not important then it does not make a sense to push it. > > I would like to point out that the properly variable naming could help with > understanding to function API without reading whole the function body. In my > opinion such things are important. > > Put my two cents in. > > Petr > > > > >BTW You introduced one of bad argument names in the recent > >commit 8babbeee01e67893af4828ddfc922ecac0be4197 > > > >Sumit ACKed this patch and he didn't ask to change the name. > >It would good to know his opinion and maybe he will propose > >better name. @see http://martinfowler.com/bliki/TwoHardThings.html
I do not have a better proposal. Technically any string can be use in those functions but the new names better illustrate that we use the string representations of SIDs here. Initially the patches used, based on my suggestion, domain_name#i where is stands for an integer number. During review Simo suggested to use domain-SID-first-RID which is a much more elegant solution then my original suggestion. Pavel changed the patches accordingly to use the SID based strings but didn't realize that the old variable name is now somewhat off and I didn't realize it during review as well while looking at diffs of diffs. Imo the new names Pavel suggested will help to make the code more comprehensible, so ACK. bye, Sumit > > > >LS > >_______________________________________________ > >sssd-devel mailing list > >sssd-devel@lists.fedorahosted.org > >https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org > > > > -- > Petr^4 Čech > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
