1) I think, 'status' function is not explicit enough. It returns 'True' if plugin is enabled and False if disabled. I think 'is_enable' would be a better name for the boolean function.
https://pagure.io/lib389/blob/master/f/lib389/plugins.py#_49

I totally agree with this and I have suggested it in the past as well.

Zen of Python says 'Flat is better than nested.'. I think it is also the case.
In my opinion, it is still readable, if we use DSLdapObject functions directly in our code (plugin.display_attr('memberofattr') still looks good). So no need for this nested methods (with one code line inside).

I see your point. I think I don't like either approach but it has to be done some way. I would be happy to apply the changes however if others agree on this.

is_enabled: Yes I'm happy with this change.

It is not explicit enough in most cases (get_attr for get_attr_val - we have also get_attr_vals, get_attrs_vals, get_all_attrs, etc.) The same case for things like 'remove_unique_subtre()' (it may be not clear for the one who will read the code with this method. He will wonder: "Will it remove my subtree from the directory or just the attribute from the plugin?")

I'm not sure I quite get this comment? Can you give me an example of how you would improve this so I can understand it?

Zen of Python says 'Flat is better than nested.'. I think it is also the case.
In my opinion, it is still readable, if we use DSLdapObject functions directly in our code (plugin.display_attr('memberofattr') still looks good). So no need for this nested methods (with one code line inside).

Yes, but this exposes the call to need to understand what LDAP attributes we use. By abstracting this we can group and remove/change functions later. Does that make sense? By having these methods "named" we have an API that is documented, but if we just say "display_attr", the user doesn't know WHAT attrs exist that can be dispalyed without some ldap internal knowledge. This way may be more code, but it makes the api discoverable.

Yes, but this exposes the call to need to understand what LDAP attributes we use. By abstracting this we can group and remove/change functions later. Does that make sense? By having these methods "named" we have an API that is documented, but if we just say "display_attr", the user doesn't know WHAT attrs exist that can be dispalyed without some ldap internal knowledge. This way may be more code, but it makes the api discoverable.

Ok, if this is our goal, then I agree. Though, we need to be very precise with the method names and docstrings. For instance,

def get_attr(self): 
    return self.get_attr_val('memberofattr')
def get_entryscope(self, formatted=False):
    return self.get_attr_vals('memberofentryscope')

both of the methods have similar name structure, but one returns a list and another a string. And there are more issues, it is only an example. I propose to bring more consistency and readability (make the method names more explicit). I'll work on this when I'll work on docstrings (if no one will take it before).

Thanks for your replies anyway. :)

Well, arguable ldap everything is a list, just it may have 0, 1 or infinite elements. So the correct solution is to always return a list. :)

In this example it is like this because one attribute is single-valued and the other one is multi-valued. Wouldn't be a little confusing as well to return an iterable when someone expects one "thing" only? Maybe we could emphasize the difference in the method names instead?

I propose to bring more consistency and readability (make the method names more explicit).

How would you propose to name them?

Something else on this.

We have recently added https://pagure.io/lib389/issue/67.

So we have more to think about; Should we always return string values and let the user of the API convert them to the appropriate type? Or we should handle this inside the API?

For example in referint plugin there's referint-update-delay attribute which is guaranteed to be an int. In this case, I think the return type of Referint.get_update_delay() would make more sense to be 0 than ['0'], [0] or anything else.

Consistency does matter, but maybe in this case it makes things more complex?

I'm not sure about the naming. That's somethnig that needs thought - and action soon, we are starting to depend on this more and more.

As for the types, I think we should have this done correctly: if you do get_update_delay() you get an int, because that's it's type. However, it's valid to have multivalued int attrs, so [0] is a valid response here, as would be [0, 1] for another type ...

Okay, lets get naming consistent. We added a bunc of types recently like:

get_attr_val_utf8
get_attr_vals_bytes

Etc. Why not focus on using these? we can ignore get_attr_val / get_attr_vals for now (or rename them to internal implementation details).

For other types, IE get_update_delay, return the correct type, and comment what it is in the docstring. There is no point giving "update delay" as bytes,str and int. Only int makes sense.

Does that help?

Okay, lets get naming consistent. We added a bunc of types recently like:
get_attr_val_utf8
get_attr_vals_bytes

Etc. Why not focus on using these? we can ignore get_attr_val / get_attr_vals for now (or rename them to internal implementation details).
For other types, IE get_update_delay, return the correct type, and comment what it is in the docstring. There is no point giving "update delay" as bytes,str and int. Only int makes sense.
Does that help?

I've thought about it and I agree with you. I'll prepare patch soon enough. (First I need to deal with issues #79, #83, #84)

Thanks mate, I look forward to seeing the patch :)