Skip to content

Commit 851a435

Browse files
2bndy52bndy5
authored
refactor: use Client instance by reference (#141)
This moves an `async fn` out of a trait declaration to allow using `Client` instance passed by reference. This avoids cloning the `Client` instance which can be costly. Show origin of log statements if not from cpp-linter and ensure log messages are flushed to stdout (while mutex is locked)
1 parent 4b1b5bd commit 851a435

File tree

5 files changed

+148
-183
lines changed

5 files changed

+148
-183
lines changed

‎cpp-linter/src/common_fs/file_filter.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@ impl FileFilter {
6161
for line in read_buf.split('\n') {
6262
if line.trim_start().starts_with("path") {
6363
assert!(line.find('=').unwrap() > 0);
64-
let submodule = String::from("./") + line.split('=').last().unwrap().trim();
64+
let submodule =
65+
String::from("./") + line.split('=').next_back().unwrap().trim();
6566
log::debug!("Found submodule: {submodule}");
6667
let mut is_ignored = true;
6768
for pat in &self.not_ignored {
@@ -159,7 +160,7 @@ impl FileFilter {
159160
let mut is_hidden = false;
160161
let parent = path
161162
.components()
162-
.last()
163+
.next_back()
163164
.ok_or(anyhow!("parent directory not known for {path:?}"))?;
164165
if parent.as_os_str().to_str().unwrap().starts_with('.') {
165166
is_hidden = true;

‎cpp-linter/src/logger.rs‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
//! A module to initialize and customize the logger object used in (most) stdout.
22
3-
use std::env;
3+
use std::{
4+
env,
5+
io::{stdout, Write},
6+
};
47

58
use colored::{control::set_override, Colorize};
6-
use log::{Level, LevelFilter, Metadata, Record};
9+
use log::{Level, LevelFilter, Log, Metadata, Record};
710

811
#[derive(Default)]
912
struct SimpleLogger;
@@ -21,21 +24,43 @@ impl SimpleLogger {
2124
}
2225
}
2326

24-
impl log::Log for SimpleLogger {
27+
impl Log for SimpleLogger {
2528
fn enabled(&self, metadata: &Metadata) -> bool {
2629
metadata.level() <= log::max_level()
2730
}
2831

2932
fn log(&self, record: &Record) {
33+
let mut stdout = stdout().lock();
3034
if record.target() == "CI_LOG_GROUPING" {
3135
// this log is meant to manipulate a CI workflow's log grouping
32-
println!("{}", record.args());
36+
writeln!(stdout, "{}", record.args()).expect("Failed to write log command to stdout");
37+
stdout
38+
.flush()
39+
.expect("Failed to flush log command to stdout");
3340
} else if self.enabled(record.metadata()) {
34-
println!(
35-
"[{}]: {}",
36-
Self::level_color(&record.level()),
37-
record.args()
38-
);
41+
let module = record.module_path();
42+
if module.is_none_or(|v| v.starts_with("cpp_linter")) {
43+
writeln!(
44+
stdout,
45+
"[{}]: {}",
46+
Self::level_color(&record.level()),
47+
record.args()
48+
)
49+
.expect("Failed to write log message to stdout");
50+
} else {
51+
writeln!(
52+
stdout,
53+
"[{}]{{{}:{}}}: {}",
54+
Self::level_color(&record.level()),
55+
module.unwrap(), // safe to unwrap here because the None case is caught above
56+
record.line().unwrap_or_default(),
57+
record.args()
58+
)
59+
.expect("Failed to write detailed log message to stdout");
60+
}
61+
stdout
62+
.flush()
63+
.expect("Failed to flush log message to stdout");
3964
}
4065
}
4166

‎cpp-linter/src/rest_api/github/mod.rs‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use reqwest::{
1616
};
1717

1818
// project specific modules/crates
19-
use super::{RestApiClient, RestApiRateLimitHeaders};
19+
use super::{send_api_request, RestApiClient, RestApiRateLimitHeaders};
2020
use crate::clang_tools::clang_format::tally_format_advice;
2121
use crate::clang_tools::clang_tidy::tally_tidy_advice;
2222
use crate::clang_tools::ClangVersions;
@@ -148,14 +148,9 @@ impl RestApiClient for GithubApiClient {
148148
None,
149149
Some(diff_header),
150150
)?;
151-
let response = Self::send_api_request(
152-
self.client.clone(),
153-
request,
154-
self.rate_limit_headers.to_owned(),
155-
0,
156-
)
157-
.await
158-
.with_context(|| "Failed to get list of changed files.")?;
151+
let response = send_api_request(&self.client, request, &self.rate_limit_headers)
152+
.await
153+
.with_context(|| "Failed to get list of changed files.")?;
159154
if response.status().is_success() {
160155
Ok(parse_diff_from_buf(
161156
&response.bytes().await?,

‎cpp-linter/src/rest_api/github/specific_api.rs‎

Lines changed: 16 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use crate::{
1717
cli::{FeedbackInput, LinesChangedOnly},
1818
common_fs::{FileFilter, FileObj},
1919
git::parse_diff_from_buf,
20-
rest_api::{RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT},
20+
rest_api::{send_api_request, RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT},
2121
};
2222

2323
use super::{
@@ -94,14 +94,9 @@ impl GithubApiClient {
9494
while let Some(ref endpoint) = url {
9595
let request =
9696
Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?;
97-
let response = Self::send_api_request(
98-
self.client.clone(),
99-
request,
100-
self.rate_limit_headers.clone(),
101-
0,
102-
)
103-
.await
104-
.with_context(|| "Failed to get paginated list of changed files")?;
97+
let response = send_api_request(&self.client, request, &self.rate_limit_headers)
98+
.await
99+
.with_context(|| "Failed to get paginated list of changed files")?;
105100
url = Self::try_next_page(response.headers());
106101
let files_list = if self.event_name != "pull_request" {
107102
let json_value: PushEventFiles = serde_json::from_str(&response.text().await?)
@@ -233,14 +228,7 @@ impl GithubApiClient {
233228
Some(serde_json::json!(&payload).to_string()),
234229
None,
235230
)?;
236-
match Self::send_api_request(
237-
self.client.clone(),
238-
request,
239-
self.rate_limit_headers.to_owned(),
240-
0,
241-
)
242-
.await
243-
{
231+
match send_api_request(&self.client, request, &self.rate_limit_headers).await {
244232
Ok(response) => {
245233
Self::log_response(response, "Failed to post thread comment").await;
246234
}
@@ -270,13 +258,7 @@ impl GithubApiClient {
270258
while let Some(ref endpoint) = comments_url {
271259
let request =
272260
Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?;
273-
let result = Self::send_api_request(
274-
self.client.clone(),
275-
request,
276-
self.rate_limit_headers.to_owned(),
277-
0,
278-
)
279-
.await;
261+
let result = send_api_request(&self.client, request, &self.rate_limit_headers).await;
280262
match result {
281263
Err(e) => {
282264
log::error!("Failed to get list of existing thread comments: {e:?}");
@@ -330,11 +312,10 @@ impl GithubApiClient {
330312
None,
331313
None,
332314
)?;
333-
match Self::send_api_request(
334-
self.client.clone(),
315+
match send_api_request(
316+
&self.client,
335317
req,
336-
self.rate_limit_headers.to_owned(),
337-
0,
318+
&self.rate_limit_headers,
338319
)
339320
.await
340321
{
@@ -391,12 +372,7 @@ impl GithubApiClient {
391372
// if we got here, then we know that it is a self.pull_request is a valid value
392373
.join(self.pull_request.to_string().as_str())?;
393374
let request = Self::make_api_request(&self.client, url.as_str(), Method::GET, None, None)?;
394-
let response = Self::send_api_request(
395-
self.client.clone(),
396-
request,
397-
self.rate_limit_headers.clone(),
398-
0,
399-
);
375+
let response = send_api_request(&self.client, request, &self.rate_limit_headers);
400376

401377
let url = Url::parse(format!("{}/", url).as_str())?.join("reviews")?;
402378
let dismissal = self.dismiss_outdated_reviews(&url);
@@ -462,22 +438,15 @@ impl GithubApiClient {
462438
dismissal.await?; // free up the `url` variable
463439
let request = Self::make_api_request(
464440
&self.client,
465-
url.clone(),
441+
url,
466442
Method::POST,
467443
Some(
468444
serde_json::to_string(&payload)
469445
.with_context(|| "Failed to serialize PR review to json string")?,
470446
),
471447
None,
472448
)?;
473-
match Self::send_api_request(
474-
self.client.clone(),
475-
request,
476-
self.rate_limit_headers.clone(),
477-
0,
478-
)
479-
.await
480-
{
449+
match send_api_request(&self.client, request, &self.rate_limit_headers).await {
481450
Ok(response) => {
482451
if !response.status().is_success() {
483452
Self::log_response(response, "Failed to post a new PR review").await;
@@ -496,13 +465,7 @@ impl GithubApiClient {
496465
while let Some(ref endpoint) = url_ {
497466
let request =
498467
Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?;
499-
let result = Self::send_api_request(
500-
self.client.clone(),
501-
request,
502-
self.rate_limit_headers.clone(),
503-
0,
504-
)
505-
.await;
468+
let result = send_api_request(&self.client, request, &self.rate_limit_headers).await;
506469
match result {
507470
Err(e) => {
508471
log::error!("Failed to get a list of existing PR reviews: {e:?}");
@@ -538,11 +501,10 @@ impl GithubApiClient {
538501
Some(REVIEW_DISMISSAL.to_string()),
539502
None,
540503
) {
541-
match Self::send_api_request(
542-
self.client.clone(),
504+
match send_api_request(
505+
&self.client,
543506
req,
544-
self.rate_limit_headers.clone(),
545-
0,
507+
&self.rate_limit_headers,
546508
)
547509
.await
548510
{

0 commit comments

Comments
 (0)