On 10/07/2015 07:10 PM, Pavel Reichl wrote:
> We can use either mine (after modifications) or yours.
I suppose yours are closer to be acked so we should probably use yours.
I'm not so sure, but alright :)
All valid comments, but I will ignore them for now as I suppose this
patch
will be superseded by yours.
Alright.
> Just checking a string for truth wouldn't be sufficient,
because an empty
> string is False and we might need to pass it sometimes. So, I think, we need
> to test against None here and above.
Thanks, my python skills got a bit rusty I suppose :-)
Mine were never sharp, but I'm generally vary of a language's concept of
truth (seen many strange ones), so I checked :)
> I really don't like how we keep copying this stuff around.
I'll look into
> making a module with these.
OK, should I postpone amending the patch until your patch is in master or
should I do it as part of this patch set?
I think it will be more efficient for you to keep working on your patch as is,
in this part, and if I manage to produce something, then we can resync.
> Could you please add descriptions for all functions, especially
these two?
Sure. I wasn't sure about those functions. Intention was to verify that
state of overrides is the same after sequence of commands - for example just
before we call 'sss_override user-export' and then after we call
'sss_override user-import'. I wanted to avoid copy&paste approach. But
wasn't sure how to find out a descriptive name. Would something like
check_user_override_state_1() be better name?
I can't really tell, since I haven't dug into what the tests actually do yet.
So far I'm just reacting to somewhat generically-named functions without
descriptions. I'm reserving the functional review until it's easier :)
Thanks for taking my comments into account :)!
Nick