-
Notifications
You must be signed in to change notification settings - Fork 32
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
Start review scary rust code from decode_github_content
:
#229
base: main
Are you sure you want to change the base?
Conversation
- keep old `deprecated` function - use normal `fn` args instead of `struct` - avoid long names - add `rustfmt.toml` and deny clippy lints
pub struct DecodeGithubContentArgument<'a> { | ||
pub content: &'a String, | ||
} | ||
|
||
pub fn decode_github_content(argument: &DecodeGithubContentArgument<'_>) -> Option<String> { |
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.
You don't need to pass struct
by ref, it just adds another one ref level for each of its fields:
content
will be&&String
|
||
#[deprecated] | ||
pub struct DecodeGithubContentArgument<'a> { | ||
pub content: &'a String, |
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.
To pass string as view you have to decide:
- you should pass as
&str
if you want only read some data - pass as
String
if you want to use allocated space in this string
let DecodeGithubContentArgument { | ||
content | ||
} = argument; |
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 mut content_without_github_shit = content.as_bytes().to_owned(); | ||
content_without_github_shit.retain(|b| !b" \n\t\r\x0b\x0c".contains(b)); | ||
let decoded_content = base64::engine::general_purpose::STANDARD | ||
.decode(&content_without_github_shit).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.
Unwrapping it here is bad, this function returns Option
.
content_without_github_shit.retain(|b| !b" \n\t\r\x0b\x0c".contains(b)); | ||
let decoded_content = base64::engine::general_purpose::STANDARD | ||
.decode(&content_without_github_shit).unwrap(); | ||
Some(String::from_utf8_lossy(&decoded_content).into_owned()) |
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.
base64
always use ASCII chars – String::from_utf8_lossy
is unnecessary
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.
Cow::into_owned
also is bad
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.
Some notes for @FreePhoenix888
extern crate core; | ||
#![deny(clippy::all, clippy::perf)] // `clippy::perf` will teach you to write good code always | ||
|
||
pub(crate) use anyhow::Result; |
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.
anyhow::Result
supports custom errors, so we can override std::Result
by it
@FreePhoenix888 where is your review |
deprecated
functionfn
args instead ofstruct
rustfmt.toml
and deny clippy lints