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?= <[email protected]> >>>>>>>>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 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
