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

Address review comments #67

Open
5 of 13 tasks
Ch00k opened this issue Jan 3, 2021 · 0 comments
Open
5 of 13 tasks

Address review comments #67

Ch00k opened this issue Jan 3, 2021 · 0 comments

Comments

@Ch00k
Copy link
Owner

Ch00k commented Jan 3, 2021

  • Your lib.rs can be divided into smaller modules.
  • Maybe trait Entity should be sealed?
  • fn parent_kind_name(&self) -> Option - maybe return Option<&str>?
  • Some functions on Entity trait can be implemented inside impl dyn Entity, to disallow override and detach dependency from implementor. https://radicle.community/t/rust-s-impl-dyn-trait-syntax/102
  • Using pub fields may render future changes to structs incompatible with previous major version of the lib (see #[non_exhaustive] https://doc.rust-lang.org/reference/attributes/type_system.html).
  • You can apply #[serde(rename_all = "PascalCase")] to more structures instead of manual renaming.
  • You still have TODOs in your code. Maybe resolve them or move into github issues?
  • You can allow user to pass a &str as an argument where you need a String with arg: impl Into and acquire a String with arg.into() (in fn auth, for example).
  • You should accept Read and Write by value, and not reference, see https://rust-lang.github.io/api-guidelines/interoperability.html#c-rw-value
  • timeout != None => timeout.is_some() or if you need to access inner value if let Some(to) = timeout {
  • I think Error could be better. What errors typical user of your crate would expect and in what form? How much details can you omit? You can hide pub fields, and instead lend message with Display.
  • Option - you can use an enum inside of your Error to encapsulate different errors. Also, consider source https://doc.rust-lang.org/std/error/trait.Error.html#method.source
  • You can move integration tests from lib.rs to /tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant