-
Notifications
You must be signed in to change notification settings - Fork 0
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(#17): register_user, Storage redesigned to struct #61
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #61 +/- ##
==========================================
- Coverage 58.82% 52.17% -6.65%
==========================================
Files 5 7 +2
Lines 17 23 +6
==========================================
+ Hits 10 12 +2
- Misses 7 11 +4 ☔ View full report in Codecov by Sentry. |
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.
@h1alexbel take a look at my comments below, please
server/src/objects/user.rs
Outdated
fn returns_username() -> Result<()> { | ||
let expected = "jeff"; | ||
let jeff = User::new(String::from(expected)); | ||
assert_eq!( |
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.
@h1alexbel might be we should add hamcrest
to project?
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.
@l3r8yJ good idea. Let's try it
server/src/xml/storage.rs
Outdated
"<root>\ | ||
<github><users/></github>\ | ||
</root>" |
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.
@h1alexbel i think we can move it to some constant
@l3r8yJ take a look again, please |
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.
@h1alexbel take a look, please
#[allow(unused_imports)] | ||
#[macro_use] | ||
extern crate hamcrest; |
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.
better to move it to mod 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.
@l3r8yJ for some reason I can't move it to the tests, it's only allowed in lib.rs
or main.rs
impl User { | ||
pub async fn save(self) -> Result<()> { | ||
info!("registering user @{}", self.username); | ||
let xml = to_string(&self).unwrap(); |
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.
let's avoid such #unwrap
calls, can you set clippy or create puzzle for it?
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.
@l3r8yJ I will create one
server/src/xml/storage.rs
Outdated
pub(crate) path: String, | ||
} | ||
|
||
const INIT_XML: &str = "<root>\ |
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.
better to use multiline string
@l3r8yJ updated |
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.
@h1alexbel looks good to me!
@l3r8yJ take a look, please
closes #17
PR-Codex overview
Focus: Add user registration functionality and improve XML storage handling.
Detailed summary
register_user
module for user registration.hamcrest
.