On Fri, Oct 02, 2015 at 03:34:30PM +0200, Michal Židek wrote: > On 10/02/2015 07:44 AM, Lukas Slebodnik wrote: > >On (01/10/15 15:41), Michal Židek wrote: > >>On 10/01/2015 03:08 PM, Lukas Slebodnik wrote: > >>>On (01/10/15 13:56), Michal Židek wrote: > >>>>On 09/16/2015 03:56 PM, Michal Židek wrote: > >>>>>On 09/15/2015 04:03 PM, Jakub Hrozek wrote: > >>>>>>On Tue, Sep 15, 2015 at 03:53:41PM +0200, Michal Židek wrote: > >>>>>>>On 09/14/2015 05:43 PM, Jakub Hrozek wrote: > >>>>>>>>On Wed, Sep 09, 2015 at 02:43:59PM +0200, Michal Židek wrote: > >>>>>>>>>Hi, > >>>>>>>>> > >>>>>>>>>patch for ticket > >>>>>>>>>https://fedorahosted.org/sssd/ticket/2673 > >>>>>>>>>is in the attachment. > >>>>>>>>> > >>>>>>>>>Thanks. > >>>>>>>>>Michal > >>>>>>>> > >>>>>>>>> From 7c454bc2a737be05068418a5eef7fe9446bb5fa8 Mon Sep 17 00:00:00 > >>>>>>>>>2001 > >>>>>>>>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> > >>>>>>>>>Date: Wed, 9 Sep 2015 14:37:48 +0200 > >>>>>>>>>Subject: [PATCH] util: Include disabled domains in link_forest_roots > >>>>>>>>> > >>>>>>>>>Ticket: > >>>>>>>>>https://fedorahosted.org/sssd/ticket/2673 > >>>>>>>>>--- > >>>>>>>>> src/db/sysdb_subdomains.c | 6 +++--- > >>>>>>>>> src/tests/cmocka/test_utils.c | 3 +++ > >>>>>>>>> src/util/domain_info_utils.c | 21 ++++++++++++++++++--- > >>>>>>>>> src/util/util.h | 3 +++ > >>>>>>>>> 4 files changed, 27 insertions(+), 6 deletions(-) > >>>>>>>> > >>>>>>>>The patch looks good but whenever I see us adding more and more > >>>>>>>>boolean > >>>>>>>>switches, I wonder if we should just use flags instead? > >>>>>>>> > >>>>>>>>This: > >>>>>>>> get_next_domain_ex(d, GND_USE_DISABLED | GND_DESCEND); > >>>>>>>>Reads quite a bit easier to me than: > >>>>>>>> get_next_domain_ex(d, true, false); > >>>>>>>> > >>>>>>>>Also, bonus point are acquired next time we add a new flag, because > >>>>>>>>not > >>>>>>>>all callers of the function must be converted.. > >>>>>>>> > >>>>>>>>What do you think? > >>>>>>> > >>>>>>>I think this is very good point. It will also give us implicit > >>>>>>>boolean parameter value (if not set, it is automatically false) > >>>>>>>which improves readability a lot. I wander if it is good to add > >>>>>>>the _ex function in this case. Would you agree if I changed the > >>>>>>>original get_next_domain to use flags? I know, it needs to be changed > >>>>>>>on more places, but I think such small refactoring makes more > >>>>>>>sence than adding _ex function with flags. > >>>>>>> > >>>>>>>Or is this what you were saying? I am not sure because > >>>>>>>you used the _ex function in your example of "nice" > >>>>>>>usage. > >>>>>> > >>>>>>If we have a test for different usages of the flag-based function, then > >>>>>>I guess it would be better than keeping the existing get_next_domain > >>>>>>function. > >>>>>> > >>>>>>We can also split master from sssd-1-13 already.. > >>>>>> > >>>>>>btw the flag names were completely pulled out of thin air, feel free to > >>>>>>suggest better ones. > >>>>> > >>>>>I split the refactoring of get_next_domain to separate patch. > >>>>>The second patch is just one liner that switches on the > >>>>>disabled domains in link_forest_roots. > >>>>> > >>>>>Michal > >>>>> > >>>> ____ _ _ __ __ ____ > >>>>__/\_| __ )| | | | \/ | _ \__/\__ > >>>>\ / _ \| | | | |\/| | |_) \ / > >>>>/_ _\ |_) | |_| | | | | __//_ _\ > >>>> \/ |____/ \___/|_| |_|_| \/ > >>>> > >>>>(low priority, just keeping the thread alive :) ) > >>>Would you mind to write unit tests for get_next_domain? > >>> > >>>LS > >> > >>It does have unit tests. > >> > >Let me rephrase your answer. > >The tests has beed changed. > > > >But new functionality is not test. > >There is a change in test_utils.s > >>- dom = get_next_domain(test_ctx->dom_list, true); > >>+ dom = get_next_domain(test_ctx->dom_list, SSS_GND_DESCEND); > >> assert_null(dom); > >>+ > >>+ dom = get_next_domain(test_ctx->dom_list, > >>+ SSS_GND_DESCEND | SSS_GND_INCLUDE_DISABLED); > >>+ assert_non_null(dom); > >But assert_non_null cannot be considered as a valid test for new feature. > > > >So would you mind to extend tests for SSS_GND_INCLUDE_DISABLED? > >It ould be god to se what happed if SSS_GND_INCLUDE_DISABLED is used without > >the flag SSS_GND_DESCEND. Or which domain will be returned in different > >cases. > > > >You should consider a new unit test as an additional documentation. > >So if new developer will not be sure hot to use it it will be obvious from > >unit test. This is a reason why it might be better to write separate > >test for new feature in separate patch. Because the current patch > >mostly convert old code to the new style 's/true/SSS_GND_DESCEND/' > >'s/false/0/' > >(In most cases, I prefer unit test to be part of new small features but > >sometimes it's better to write unit test in separate patch. > > > >LS > > Ok. I removed the test changes from the first patch and added > enhanced tests in the new patch. > > Michal >
Can you rebase the patches atop master, please? I will then review them. But from casual scrolling from the patches, the changes look OK to me. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel