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

Start review scary rust code from decode_github_content: #229

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions rust/update-conan-recipe-and-create-pull-request/rustfmt.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
error_on_line_overflow = true
error_on_unformatted = true
version = "Two"

imports_granularity = "One"
use_small_heuristics = "Max"
Original file line number Diff line number Diff line change
@@ -1,17 +1,29 @@
use base64::Engine;
use crate::add_version_to_conandata_yml::AddVersionToConandataYmlArgument;
use {crate::Result, base64::DecodeError};

#[deprecated]
pub struct DecodeGithubContentArgument<'a> {
pub content: &'a String,
Copy link
Member Author

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

}

pub fn decode_github_content(argument: &DecodeGithubContentArgument<'_>) -> Option<String> {
Copy link
Member Author

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

let DecodeGithubContentArgument {
content
} = argument;
Comment on lines -9 to -11
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can match in fn sig - any let .. = .. and ..: Type is always pattern matching
let expr and fn arg

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();
Copy link
Member Author

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.

Some(String::from_utf8_lossy(&decoded_content).into_owned())
Copy link
Member Author

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

Copy link
Member Author

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

}
#[deprecated(note = "keep `decode_github_content` for soft migration into `decode`")]
pub fn decode_github_content(
&DecodeGithubContentArgument { content }: &DecodeGithubContentArgument,
) -> Option<String> {
decode(
// This leads to unnecessary cloning
content.clone(),
)
.ok()
}

// use `String` because it is useful for `octorust` types
pub fn decode(mut content: String) -> Result<String, DecodeError> {
// base64 alphabet is always utf8 safe
Ok(unsafe {
// `base64::decode` is deprecated but it is very simple use case
String::from_utf8_unchecked(base64::decode({
content.retain(|b| !b.is_whitespace());
content.into_bytes()
})?)
})
}
4 changes: 3 additions & 1 deletion rust/update-conan-recipe-and-create-pull-request/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
extern crate core;
#![deny(clippy::all, clippy::perf)] // `clippy::perf` will teach you to write good code always

pub(crate) use anyhow::Result;
Copy link
Member Author

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


use std::error::Error;
use std::iter::zip;
Expand Down