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?=
<mzidek(a)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