Review our APIs against the Rust API guidelines checklist

[coryan]

We should compare our APIs against the Rust API Guidelines Checklist and:

Document any deviations that we do not want to fix and why

2. Interoperability (crate interacts nicely with other library functionality)

• Data structures implement Serde's Serialize, Deserialize (C-SERDE)
  • The motivation is that types are "saveable".
  • Note that our message types have this property.
  • I cannot think of another type where this would be useful. e.g. saving a ClientConfig does not seem that useful. And if we do it, we would have to think about backwards compatibility for the serialization/deserialization. ew.

6. Flexibility (crate supports diverse real-world use cases)

• Traits are object-safe if they may be useful as a trait object (C-OBJECT)
  • docs.rs will call out the traits that are not dyn-compatible. So we are at least predictable.
    • https://docs.rs/google-cloud-kms-v1/latest/google_cloud_kms_v1/stubs/trait.Autokey.html#dyn-compatibility
  • I do not think there is much utility to having these as trait objects though.
  • Applications can use the client structs, not the stub traits.

8. Dependability (crate is unlikely to do the wrong thing)

• Functions validate their arguments (C-VALIDATE)
  • It is infeasible/impossible to verify that a request is valid for any particular service on the client side.
  • We could add newtypes for resources. e.g. a Project which knows its name is "projects/<project>".
    • These expand the API surface, opening us up to more breaks
    • I think we would keep our request builders accepting strings for resource names.
    • I think we would not accept these new types, because that might be too restrictive.
    • We could add them later
  • Some fields can be validated on the client side.
    • Tracked in AIP-4231: Decide if we want to provide helpers for resource path construction #520

10. Future proofing (crate is free to improve without breaking users' code)

• Structs have private fields (C-STRUCT-PRIVATE)
  • The flexibility gains in changing representations come at the cost of usability for the structs. We do not think this trade off is worth it (at least for our generated messages, many of which are used in responses from the server).
  • See Consider making all fields private with accessors #1382 for more details/discussion.
  • I still think this is a nice to have for handwritten types (where the ability to move-out fields is less important).

Document any deviations we want to fix and result in breaking changes

1. Naming (crate aligns with Rust naming conventions)

• Ad-hoc conversions follow as_, to_, into_ conventions (C-CONV)
  • LRO, Paginator's to_stream methods should be into_stream because they are cheap. They consume the object, without copying.
• Getter names follow Rust convention (C-GETTER)
  • auth's get_token(), get_headers(), get_universe_domain() might want to drop the get_ prefix.

other inconsistencies:

• cleanup ClientConfig setters #1622
• Make Paginator a trait #1623

4. Documentation (crate is abundantly documented)

• Rustdoc does not show unhelpful implementation details (C-HIDDEN)
  • We are working towards hiding all implementation details.
  • But we probably need to audit some of the impls that are inevitably public, but irrelevant.
  • I suspect we will have to say: "if something does not have docs, it is not in our public API.", which is why I categorize hiding documentation as a break.

7. Type safety (crate leverages the type system effectively)

• Builders enable construction of complex values (C-BUILDER)
  • We have this almost everywhere. We use builders to make RPCs.
  • We have builders for option types, which we pass to a client, but maybe we want the clients to be builders?
    • Tracked in Finalize design for client initialization #1548

10. Future proofing (crate is free to improve without breaking users' code)

• Sealed traits protect against downstream implementations (C-SEALED)
  • Tracked in Proposal: "seal" common traits #1609

Document any deviations we want to fix, but won't result in breaking changes (e.g. things that are purely additions to the APIs, or samples)

4. Documentation (crate is abundantly documented)

• Crate level docs are thorough and include examples (C-CRATE-DOC)
• All items have a rustdoc example (C-EXAMPLE)
  • There is a decent gap here
• Release notes document all significant changes (C-RELNOTES)
  • We do document significant changes, although this is not particularly easy to read
  • https://github.com/googleapis/google-cloud-rust/releases/tag/v0.3.0
  • The main thing is that our releases are for google-cloud-rust, as a whole. They do not relate to individual crates.
  • Customers likely want to know: what happened in between google-cloud-foo v1.0 and google-cloud-foo v1.1. It would be nice to attach this information to the crate google-cloud-foo somehow.
  • In general, the mapping between the repo (google-cloud-rust)'s vX and the crate (google-cloud-foo)'s vY could be more apparent.

6. Flexibility (crate supports diverse real-world use cases)

• Consider exposing the current page token in ItemPaginator #1961
  • ItemPaginator might want to return the current page token (so streams can be restarted). Exposing the current page is probably good enough.

9. Debuggability (crate is conducive to easy debugging)

• All public types implement Debug (C-DEBUG)
• (2) Types eagerly implement common traits (C-COMMON-TRAITS)
  Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, Display, Default
  • We can add these later as needed. Not breaking.

11. Necessities (to whom they matter, they really matter)

• Public dependencies of a stable crate are stable (C-STABLE)
  • We are currently good, but it would be nice to enforce this via a process. Tracked in Prevent unstable dependencies in our public API #1108

---

DONE

1. Naming (crate aligns with Rust naming conventions)

• Casing conforms to RFC 430 (C-CASE)
  • rustc linters + cargo clippy -- --deny warnings should catch these.
• Feature names are free of placeholder words (C-FEATURE)
• Names use a consistent word order (C-WORD-ORDER)
• Methods on collections that produce iterators follow iter, iter_mut, into_iter (C-ITER)
• Iterator type names match the methods that produce them (C-ITER-TY)

2. Interoperability (crate interacts nicely with other library functionality)

• Error types are meaningful and well-behaved (C-GOOD-ERR)
• Binary number types provide Hex, Octal, Binary formatting (C-NUM-FMT)
• Generic reader/writer functions take R: Read and W: Write by value (C-RW-VALUE)
• Collections implement FromIterator and Extend (C-COLLECT)
  • We do not have any proper collection types at the moment.
• Types are Send and Sync where possible (C-SEND-SYNC)
• Conversions use the standard traits From, AsRef, AsMut (C-CONV-TRAITS)
  • I am only seeing the gax Status types as having public conversion functions, and these are correct.
  • And our wkt::Duration is integrated with chrono, and time types.
  • And our enum messages can be converted from i32.
  • Most of our public types do not require any sort of conversions to each other, or other types.

3. Macros (crate presents well-behaved macros)

4. Documentation (crate is abundantly documented)

• Examples use ?, not try!, not unwrap (C-QUESTION-MARK)
  • So far so good. We run cargo doc, so I think that build would likely fail if we ever panic inappropriately instead of erroring.
• Prose contains hyperlinks to relevant things (C-LINK)
• Cargo.toml includes all common metadata (C-METADATA)
  authors, description, license, homepage, documentation, repository, keywords, categories
• Function docs include error, panic, and safety considerations (C-FAILURE)
  • Our clients are fairly dumb. Something has gone horribly wrong in an underlying layer if our API is panicking.
  • We do document when our types are shared (i.e. they hold an Arc<> internally, so applications do not need to wrap them).

5. Predictability (crate enables legible code that acts how it looks)

• Smart pointers do not add inherent methods (C-SMART-PTR)
• Conversions live on the most specific type involved (C-CONV-SPECIFIC)
• Functions with a clear receiver are methods (C-METHOD)
• Functions do not take out-parameters (C-NO-OUT)
• Operator overloads are unsurprising (C-OVERLOAD)
• Only smart pointers implement Deref and DerefMut (C-DEREF)
• Constructors are static, inherent methods (C-CTOR)

6. Flexibility (crate supports diverse real-world use cases)

• Caller decides where to copy and place data (C-CALLER-CONTROL)
• Functions minimize assumptions about parameters by using generics (C-GENERIC)
  • We are in the habit of accepting Into<T>s.
  • Even if we miss a spot, generalizing the input will not result in a break.

7. Type safety (crate leverages the type system effectively)

• Newtypes provide static distinctions (C-NEWTYPE)
• Types for a set of flags are bitflags, not enums (C-BITFLAG)
• Arguments convey meaning through types, not bool or Option (C-CUSTOM-TYPE)
  • Maybe tracing_enabled() should be a more complicated type? Then we would say set_tracing(TracingConfig), and not have to add an API later for fine-grained tracing.
  • Eh. I am going to rule that this is not a big deal.

8. Dependability (crate is unlikely to do the wrong thing)

• Destructors never fail (C-DTOR-FAIL)
• Destructors that may block have alternatives (C-DTOR-BLOCK)

9. Debuggability (crate is conducive to easy debugging)

• Debug representation is never empty (C-DEBUG-NONEMPTY)

10. Future proofing (crate is free to improve without breaking users' code)

• Newtypes encapsulate implementation details (C-NEWTYPE-HIDE)
  • e.g. see the wkt implementations.
• Data structures do not duplicate derived trait bounds (C-STRUCT-BOUNDS)
  • I have not seen instances of this.
  • Our client builders are: RequestBuilder<R: Default>, but I think that is different.

11. Necessities (to whom they matter, they really matter)

• Crate and its dependencies have a permissive license (C-PERMISSIVE)

[c-git]

The current link to the guidelines is https://www.google.com/url?q=https://rust-lang.github.io/api-guidelines/checklist.html but https://rust-lang.github.io/api-guidelines/checklist.html would skip the redirect

[coryan]

The current link to the guidelines is https://www.google.com/url?q=https://rust-lang.github.io/api-guidelines/checklist.html but https://rust-lang.github.io/api-guidelines/checklist.html would skip the redirect

Fixed.

[coryan]

@dbolduc are the get_* accessors for oneof fields Okay?

[dbolduc]

@dbolduc are the get_* accessors for oneof fields Okay?

Argh, don't think so. Good catch. #1803

[dbolduc]

I think any outstanding work for this is tracked in its own issue. Closing.