On 04/27/2015 05:56 PM, Nikolai Kondrashov wrote:
> On 04/24/2015 01:44 PM, Nikolai Kondrashov wrote:
>> Here are some examples of using the module:
>>
>> # Assert getpwnam("user1") returns an entry with uid == 1001 and
>> # gid == 2001
>> ent.passwd_assert_by_name("user1", dict(uid=1001, gid=2001))
>
> It will be better if all assert functions begin with the
> prefix assert_ (or alternatively you can put them to separate
> module called ci_assert or something similar, but maybe prefixes
> are enough)
I wanted to have database name in front, but I think I can put the verb
forward instead. It might make the code easier to read, indeed.
> and they should include the condition that needs to
> be met as part of their identifier. So this function should
> be something like:
>
> assert_user_by_name_has_attributes("user1", dict(uid=1001, gid=2001))
>
> or with uid:
>
> assert_user_by_uid_has_attributes(1001, dict(name="user1", gid=2001))
The problem is, "has_attributes" would only be correct for users. For
groups
we would have to use something else after we add partial matching of group
members. I think we can use "matches" instead, as in
"assert_user_by_uid_matches", as in "matches the pattern". We would
have to
use "matches" with most of the functions, though. So, if all of them just
check for pattern matching, maybe we shouldn't repeat that and omit
"matches"
altogether?
Still, switching to using specific functions can fix that (see below).
> The function to test just for existence of a user could look
> like this:
>
> assert_user_by_name_exists("user1")
There is no actual libc interface to check for user existence, so I think
having an assertion for that will be misleading. If you don't care what
attributes a user has, and only want to check that it exists, just pass an
empty pattern:
assert_passwd_by_name("user1", {})
BTW, I wouldn't like to change "passwd" to "user". I think it
will be more
straightforward to simply use NSS database names for all entry types.
assert_passwd_by_name is a good name and it is probably better than
the _has_attributes that I suggested. My strongest request was that
all assert functions start with the prefix assert_ .
> Do not worry too much if the identifiers get longer. Sometimes
> it is better to sacrifice the keyboard lifetime for the sake of
> better readability :) .
More verbose interface might be easier to read the first few times, but
becomes a burden both to read (more text to scan) and to write (more
text to
type and make mistakes in), after you learn it.
I tend to disagree here. It is IMO much greater burden if I need
to check internals of functions just to get an idea
of what the function is doing.
>> I attempted to describe the logic behind the generic matching
function
>> "diff", but please ask if anything is not clear. There is one
particular
>> flaw to it, though: there is no way to match a list partially, e.g. to
>> match for presence of just a few group members, instead of all. I
>> think we
>> can employ tuples for these. I.e. say that a tuple pattern matches a
>> list
>> if all tuple items match at least one list item, but not necessarily the
>> other way around.
>
> It will be better to remove this limitation, but not with the
> tuples.
> If think it is not good to have change in behaviour based on whether
> the argument is tuple or list. A better approach would be to have
> additional boolean argument in diff(), named for example match_all with
> default set to False (or True). If the parameter is True, then
> all returned entries must match some pattern, if not, it is enough
> if each given pattern matches some entries in the returned list of
> entries. Would that work for you?
I would very much like to avoid this solution. It would be all-or-nothing
(i.e. no mixing different conditions in one call, no matter how high-level)
and would increase the code size by requiring passing that variable through
the stack of many functions.
Well, the parameter match_all would have default value (I think False
would be a good default) so the user will not have to pass it if
he is OK with the default behaviour. And the stack of the functions
is not very high (2) so I do not see this as a big problem.
We already have different matching logic for dictionaries and lists. Adding
tuples doesn't make it much more complex, and keeps the patterns
succinct and
expressive.
The difference between lists and dictionaries is visually big
enough (if the dictionary is not empty) to make sure the user will
not mistake the two and get the expected behaviour. In case
of tuples and lists, the visual difference is not so big.
I think it will be better if we get rid of this behaviour where
different parameter types do these subtle diffrencies in
behavior.
> Instead of "unmatched pattern items", it would be better to use
> something that is readable even for people who are not familiar
> with the implementation. Imagine a contributor wondering: "What
> the heck is pattern item?". Maybe something like this would be
> better:
>
> - "Expected entries with these attributes were not found:" followed by
> list of patterns that did not match any of the returned entries
>
> - "Unexpected entries found:" followed by entries that were
> not matched by any given pattern (only if argument match_all
> described above is true)
>
> Feel free to be verbose here. It will only be printed in case
> of error, so the easier to spot, the better.
I'll try to think of something better, but I suspect not much can be done
here. The problem is the "diff" function is very generic and is used to
match
everything. This is done in an attempt to write less code (so there is
less to
understand and to maintain).
At the moment, it is used to match entry (passwd/group) lists, as well as
group member lists. So, "with these attributes" wouldn't apply to group
members. Moreover, I would like to use this function to match other entry
types eventually. I.e. netgroups and services and that may make it even
more
difficult to produce non-abstract error messages, which would fit all the
uses.
This is the general problem of doing abstract (short) code - the
specifics are
shifted up the stack. Personally, I have no problem with that. I prefer
shorter and more flexible code that is easier to apply to new problems,
and I
usually adjust to the patterns required to use it easily. I understand
that it
makes it harder to read for the first few times, but I think it is worth
it in
the long term. In our case, I think we can afford it, as we're smart and
have
low developer turnover, and even outside developers need to be smart as
well,
to understand other specifics. Still, I might be miscalculating.
If you think this might be a problem after all, I can do away with the
single
matching function and make specific functions instead. I.e. have separate
functions to match various conditions of separate entry types, which would
produce more specific error messages.
However, there is one drawback to this: there will be more code,
copy-pasting,
and larger, less consistent interface to learn and maintain. I think
that it
is easier to learn what a pattern is and how it matches, once, then
learn what
each function matches against, than learn a bunch of specific functions and
their specific interfaces. However, I understand that use of such interface
might be cryptic at first.
So, would you agree to keep the interface generic and somewhat harder to
learn, but easier to keep in mind and more flexible?
Or would you prefer I make the interface more specific, with descriptive
function names, each tailored to specific task and producing specific error
messages, so easier to understand the first few times, but larger and
harder
to remember, and needing more energy to read and to type, once learned?
We can add "type" parameter to the diff function and init
the messages based on this parameter. This way we will not
have to copy-paste the code and we could use more specific
messages.
> Also one more important thing. You mix up functions that can
> be considered public API of the modules together with helper
> private functions (that would be static in C). We should
> clearly distinguish between public and private functions
> (add underscore as prefix to the private functions and maybe
> group the public ones on top of the module?)
Alright, I'll hide some of the functions that are of little use to the
outside
world, and you'll tell me if you'd like anything else hidden.
Also do not forget to rebase the patches on top of the
current master. There were some changes in the build
files recently.
Michal