Re: [pbs-devel] [RFC proxmox-backup 1/3] acl: add docs and adapt visibility

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On December 17, 2020 5:12 pm, Thomas Lamprecht wrote:
> On 17/12/2020 15:27, Fabian Grünbichler wrote:
>> document all public things, add some doc links and make some
>> previously-public things only available for test cases or within the
>> crate:
>> 
>> previously public, now private:
>> - AclTreeNode::extract_user_roles (we have extract_roles())
>> - AclTreeNode::extract_group_roles (same)
>> - AclTreeNode::delete_group_role (exists on AclTree)
>> - AclTreeNode::delete_user_role (same)
>> - AclTreeNode::insert_group_role (same)
>> - AclTreeNode::insert_user_role (same)
>> - AclTree::write_config (we have save_config())
>> - AclTree::load (we have config()/cached_config())
>> 
>> previously public, now crate-internal:
>> - AclTree::from_raw (only used by tests)
>> - split_acl_path (used by some test binaries)
>> 
> 
> thanks, thats makes it cleaner
> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> RFC to get some feedback on which rustdoc features we actually want to
>> use here, and which we want to leave out for now.
>> 
> 
> Anything specific you want comments for, I mean I only skimmed through it quickly 
> so may have overlooked stuff, but it seems like its mostly links which are new
> in usage?
> 
> As long its a stable supported rustdoc feature available by the rust version
> we use I'm all for being able to use it.
> 

yes, mainly the linking (rust 1.48 now supports proper intra-crate 
linking without having to guess the HTML structure it creates, so this 
should work nicely now).

my main concern were the following (sorry for not taking the time to 
write them up):

nice to read links in the code (i.e., as plain as possible) don't look 
nice in the generated docs, and vice-versa => which part gets priority?

e.g., the following:

    /// Returns applicable [Role] and their propagation status for a given
    /// [Authid](crate::api2::types::Authid).
    ///
    /// If the `Authid` is a [User](crate::config::user::User) that has no specific `Roles` configured on this node,
    /// applicable `Group` roles will be returned instead.

is not nice to parse while reading the code, but it will read much 
better in the generated HTML than

    /// Returns applicable [Role] and their propagation status for a given
    /// [crate::api2::types::Authid]
    ///
    /// If the `Authid` is a [crate::config::user::User] that has no specific `Roles` configured on this node,
    /// applicable `Group` roles will be returned instead.

does. according to the docs the following should also work, and might 
actually be the nicest variant:

    /// Returns applicable [Role] and their propagation status for a given
    /// [Authid].
    ///
    /// If the `Authid` is a [User] that has no specific `Roles` configured on this node,
    /// applicable `Group` roles will be returned instead.
    ///
    /// [Authid]: crate::api2::types::Authid
    /// [User]: crate::config::user::User

a second point was that for structs we have to watch out that not too 
much rustdoc special syntax slips into the generated API schema 
descriptions (thankfully, examples are usually limited to functions). we 
could try some heuristics to run on our api dump (e.g., complaining if 
we add special syntax compared to the last tracked api dump) to catch 
that, since we just need to override the #[api] description which takes 
precedence over the auto-generated one to fix it. or, we could try to 
automagically strip some stuff when using the auto-generated one.