Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adhere to Rust API Guidelines #8

Open
26 of 60 tasks
auscompgeek opened this issue Oct 1, 2018 · 11 comments
Open
26 of 60 tasks

Adhere to Rust API Guidelines #8

auscompgeek opened this issue Oct 1, 2018 · 11 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@auscompgeek
Copy link
Member

auscompgeek commented Oct 1, 2018

See https://rust-lang.github.io/api-guidelines/ for details.


  • Naming (crate aligns with Rust naming conventions)
    • Casing conforms to RFC 430 (C-CASE)
    • Ad-hoc conversions follow as_, to_, into_ conventions (C-CONV)
    • Getter names follow Rust convention (C-GETTER)
    • 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)
    • Feature names are free of placeholder words (C-FEATURE)
    • Names use a consistent word order (C-WORD-ORDER)
  • Interoperability (crate interacts nicely with other library functionality)
    • Types eagerly implement common traits (C-COMMON-TRAITS)
      • Copy, Clone, Eq, PartialEq, Hash
      • Ord, PartialOrd
      • Debug
      • Display
      • Default
    • Conversions use the standard traits From, AsRef, AsMut (C-CONV-TRAITS)
    • Collections implement FromIterator and Extend (C-COLLECT)
    • Data structures implement Serde's Serialize, Deserialize (C-SERDE)
    • Types are Send and Sync where possible (C-SEND-SYNC)
    • 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)
  • Macros (crate presents well-behaved macros)
  • Documentation (crate is abundantly documented)
    • Crate level docs are thorough and include examples (C-CRATE-DOC)
    • All items have a rustdoc example (C-EXAMPLE)
    • Examples use ?, not try!, not unwrap (C-QUESTION-MARK)
    • Function docs include error, panic, and safety considerations (C-FAILURE)
    • Prose contains hyperlinks to relevant things (C-LINK)
    • Cargo.toml includes all common metadata (C-METADATA)
      • authors, description, license, homepage, documentation, repository,
        readme, keywords, categories
    • Crate sets html_root_url attribute "https://docs.rs/CRATE/X.Y.Z" (C-HTML-ROOT)
    • Release notes document all significant changes (C-RELNOTES)
    • Rustdoc does not show unhelpful implementation details (C-HIDDEN)
  • 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)
  • Flexibility (crate supports diverse real-world use cases)
    • Functions expose intermediate results to avoid duplicate work (C-INTERMEDIATE)
    • Caller decides where to copy and place data (C-CALLER-CONTROL)
    • Functions minimize assumptions about parameters by using generics (C-GENERIC)
    • Traits are object-safe if they may be useful as a trait object (C-OBJECT)
  • Type safety (crate leverages the type system effectively)
    • Newtypes provide static distinctions (C-NEWTYPE)
    • Arguments convey meaning through types, not bool or Option (C-CUSTOM-TYPE)
    • Types for a set of flags are bitflags, not enums (C-BITFLAG)
    • Builders enable construction of complex values (C-BUILDER)
  • Dependability (crate is unlikely to do the wrong thing)
  • Debuggability (crate is conducive to easy debugging)
  • Future proofing (crate is free to improve without breaking users' code)
  • Necessities (to whom they matter, they really matter)
@Lytigas
Copy link
Collaborator

Lytigas commented Oct 14, 2018

Thanks for putting in this list. If anyone could help go through and point out problems they see that would be great; I don't have much free time for this right now.

@Lytigas Lytigas added help wanted Extra attention is needed good first issue Good for newcomers labels Oct 14, 2018
@inquisitivecrystal
Copy link
Contributor

This is up my alley, but it's a big task. So I'm going to try to do some work on it, but that doesn't mean that I'm opposed to another person helping if they're interested .

@inquisitivecrystal
Copy link
Contributor

@Lytigas In the necessities section, we are non-compliant with C-PERMISSIVE. It's listed as a necessity for good reasons: it's necessary for some people to be able to use the library. I know you disagree with my opinion here, but please seriously consider dual MIT/Apache. It's not just my recommendation, it's part of the lib team's guidelines for being a part of the rust ecosystem.

If you refuse to consider changing the license (which IMO you should), you should strike through that item on the checklist and mark it WONTFIX.

@auscompgeek
Copy link
Member Author

+1 to the note above. I feel GPLv3 isn't appropriate for a library, and given that this is essentially a derivative of a BSD project, the licensing is very odd.

@inquisitivecrystal
Copy link
Contributor

@Lytigas Any reply?

@Lytigas
Copy link
Collaborator

Lytigas commented Oct 18, 2018

For posterity I'll include here that the project has been re-licensed.

@Lytigas
Copy link
Collaborator

Lytigas commented Oct 23, 2018

One thing we'll have to get straight is whether our setters will be traditional C++ style or Rust-style get_mut() -> &mut i32.

@inquisitivecrystal
Copy link
Contributor

I'm signing off on C-METADATA.

@jmeggitt
Copy link
Contributor

@Lytigas, I think it would be better to take the rust approach since using rust is what this entire project is all about. Also, I have a feeling some clippy lints might complain about doing it the C++ way.

@auscompgeek
Copy link
Member Author

WPILib have a lot of technical debt anyway. Trying to write idiomatic Rust should help us move away from that (which is the reason behind a couple of the open PRs here).

@Cynosure-null
Copy link

I assume this issue still needs a bit of work, should I try to work on it a bit? I'm not great with Rust but this feels like a good place to learn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants