-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: app ratings endpoint #15
Conversation
@@ -0,0 +1,10 @@ | |||
mod user_tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this very much better than the approach I had previously!
Although interesting that you don't need a mod.rs
in each sub folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New to me, but apparently its been here for a while 😄 looks like since Rust 2018 its sufficient to just declare the sub modules in the parent directory. As you say, makes it nice to show everything we have in all the sub directories in one place! We can actually rename it from mod.rs if we want.
https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good! A few changes please:
- Can you run clippy locally and address the issues or disable the lint (but only when there's good reason and if we want to disable a lint then lets discuss 🙏)
- I've added some comments to address inline
Once you've made those changes send it back my way please and I'll have a more thorough tinker with the API.
2cc7926
to
66a4d4d
Compare
Only Clippy lints left are regarding not having made use of those Config structs in our code yet, but that presumably will change in time? Let me know if you are happy with how I did the documenting of the Wilson's score interval. Tried to keep it concise and < 80 char per line. |
66a4d4d
to
c2c2ef5
Compare
Excellent work. |
New app ratings endpoint and its tests