Thanks for a quick answer, Michal!
See my (short) answers below.
On 05/15/2015 07:14 PM, Michal Židek wrote:
On 05/15/2015 03:39 PM, Nikolai Kondrashov wrote:
>> One of the patches generates whitespace warnings:
>>
>> Applying: Add integration tests
>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:419: new blank line at
>> EOF.
>> +
>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:1163: new blank line
>> at EOF.
>> +
>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:1276: new blank line
>> at EOF.
>> +
>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:1535: new blank line
>> at EOF.
>
> Hmm, I was putting these there on purpose, to have each function end with
> separating whitespace for ease of re-arranging, and to have some whitespace
> below the last function when going to the end of the file, so it doesn't
> stick
> to editor decorations, making it harder to read. These are new files and
> I'm
> not adding spurious whitespace to existing files here.
>
> Is this a problem? I can remove it if it is.
I would not call it a problem, but it is better if the git
am applies without unnecessary warnings and I do not think
that the newlines in the end add any value (but it may depend
on editor). I would remove them.
Also I just realized that you use 2 blank lines between functions.
I think it is a custom to use just one in SSSD (at least this
is how it is in most of the SSSD code). IMO one is enough.
I was following PEP 8:
https://www.python.org/dev/peps/pep-0008/#blank-lines
I think for Python code it makes more sense than for C code, because Python
doesn't use braces, which add a bit of vertical air and you can use single
empty line inside functions.
Anyway I'll remove them if it is better for sssd.
> What if we try to make them simpler? Let's remove
list/uid/gid/name
> arguments.
> Just make diff/assert_passwd/group accept only patterns and always check
> everything. This way the difference between "contains" and
"contains_only"
> will make sense as we're always checking against the entry list retrieved
> by get(pw|gr)ent.
Ok, that sounds good IMO.
Great! Will do then.
>> Also make the diff function _diff, we will not use it outside
the
>> module.
>
> I'm not so sure about that. Some corner-case tests might need some special
> matching and assertions which won't be worth adding to ent.py. At least
> until
> they prove useful elsewhere. Since we can't make assertion backtrace
> start at
> arbitrary point up-stack, building functions on top of the exposed assert_*
> functions wouldn't be desirable, and using "diff" in these custom
functions
> would be more appropriate. It will also be more difficult to form coherent
> error messages on assertion failures if building on top of assert_*
> functions.
>
> Still, it will be easy to expose "diff" later if necessary, and I can do
as
> you request.
>
You are right that the function could be useful under some
circumstances, but we shall strive toward clean API, not toward
a bunch of inconsistent functions that may be useful under some
circumstances. We have this problem with our sysdb API, where
we have a whole lot of functions useful under some circumstances
and overall it is mess.
The diff function exposes too much internal dirty little secrets
that the user should not care about. For example it takes the
description map as an argument. None of the functions that
take the description map as an argument should ever be made
public. This is for internal use only, to make the asserts
produce better error messages. If we found our selves in the
situation where we would like to use the diff function
directly in the tests, it is probably time to introduce new
assert_ function to the API, that can use the diff internally.
Also the diff_passwd and diff_group should be made static.
The fact that we use some diffs is really of no importance
to the assert_ function caller.
Well, I was talking about the case where we might want to add an assert_
function which makes sense for one test file only and is too specific (at
least for the start) to be put into ent.py.
Still, if we find that a problem, we can always expose these, so let's start
with them hidden.
>> And one more thing. Use all the assert_ functions in the
example
>> tests. It will serve as example/documentation for later usage.
>
> These weren't example tests, just some actual tests to start with.
> However, I
> can add some more tests that would use these, no problem. It will take some
> more time, though.
>
Well they are the first tests that use this new code, so they
will be examples for others when writing new tests. It will
be helpful for developers to see the function in real context.
So I think it is worth the effort and the tests can be really
simple. Additionally we will test the assert_ functions
themselves this way (this is IMO important).
Alright, will do.
Nick