On 4/3/24 12:46, Max Carrara wrote: > Four overall things I want to mention: > > 1. IMO a lot of the `pub` items should eventually be documented, > preferably once the actual series is out. I don't think we need to > be as thorough as e.g. the Rust STL's documentation, but I don't > think it would hurt if the overall functionality of things was > documented. (Of course, e.g. saying that `pub fn hostname()` > "gets the hostname" isn't necessary; but you get what I mean :P )
As already mentioned in-line I am currently working on this. > 2. Constants and defaults should also be documented, simply because > it makes it easier to refer to those defaults if necessary. On top > of that, it's also more obvious if those constants / defaults ever > have to be changed for some reason. That way we would avoid > accidental semver-breakage. There's a more specific example inline. see my in-line comments in the specific patch. > 3. Would it perhaps actually make sense to use `thiserror` instead of > `anyhow`? I know we've speculated a little off list about this > already - I still am not 100% convinced that `thiserror` is > necessary, but then again, it would be quite nice in the library > crates, as you don't really need to propagate any `anyhow::Context` > anyways ... > > There's already `NftError` in proxmox-nftables that *could perhaps* > just be implemented via `thiserror`, I think. Yes, error handling is probably the one big thing that needs some overhauling. Since this was a monolithic crate that I've then extracted into 3 different crates, anyhow was used throughout the whole codebase. Not sure if thiserror is really necessary here, just like you, probably just custom error types would suffice imo. > 4. Some of the types (in particular in `proxmox-ve-config` and > `proxmox-nftables`) could use some more trait-deriving - a lot of > the structs and enums could benefit from deriving `PartialOrd`, > `Ord` and `Hash` for interoperability's sake [0]. While it's > probably unlikely that some types will ever be used as keys in a > hashmap, deriving the trait IMO doesn't hurt. > > A lot of types also implement `PartialEq` and `Eq` only for tests, > but IMO those traits could theoretically just always be implemented > for most of them. > > As this affects a lot of types I've decided to just sum this up > here by the way; if you need more concrete examples, please let me > know and I'll add respective comments inline. Good point, I will review the structs/enums and add additional derivations where applicable. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
