Skip to content

Commit fc75245

Browse files
authored
deprecate: replace --tidy-review/--format-review with --pr-review (#377)
- add `--pr-review` to control posting PR reviews - make `--tidy-review`/`--format-review` aliases of `--pr-review` for backward compatibility resolves #375 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Consolidated PR review configuration into a single unified `--pr-review` flag, replacing separate review toggles for better user clarity and simpler command-line setup. * Legacy `--tidy-review` and `--format-review` flags remain available as backward-compatible aliases to ensure existing workflows continue without interruption. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent f352be2 commit fc75245

6 files changed

Lines changed: 18 additions & 74 deletions

File tree

cpp-linter/src/clang_tools/clang_tidy.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -518,8 +518,6 @@ mod test {
518518
database_json: None,
519519
format_filter: None,
520520
tidy_filter: None,
521-
tidy_review: false,
522-
format_review: false,
523521
clang_tidy_command: Some(exe_path),
524522
clang_format_command: None,
525523
repo_root: tmp_workspace.path().to_path_buf(),

cpp-linter/src/cli/mod.rs

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -506,31 +506,20 @@ pub struct FeedbackOptions {
506506
))]
507507
pub file_annotations: bool,
508508

509-
/// Set to `true` to enable Pull Request reviews from clang-tidy.
509+
/// Set to `true` to enable Pull Request reviews.
510510
#[cfg_attr(feature = "bin", arg(
511-
short = 'd',
511+
short = 'P',
512512
long,
513513
default_value_t = false,
514514
default_missing_value = "true",
515515
num_args = 0..=1,
516516
action = ArgAction::Set,
517517
value_parser = FalseyValueParser::new(),
518518
help_heading = "Feedback options",
519+
aliases = ["format-review", "tidy-review"],
520+
short_aliases = ['d', 'm'],
519521
))]
520-
pub tidy_review: bool,
521-
522-
/// Set to `true` to enable Pull Request reviews from clang-format.
523-
#[cfg_attr(feature = "bin", arg(
524-
short = 'm',
525-
long,
526-
default_value_t = false,
527-
default_missing_value = "true",
528-
num_args = 0..=1,
529-
action = ArgAction::Set,
530-
value_parser = FalseyValueParser::new(),
531-
help_heading = "Feedback options",
532-
))]
533-
pub format_review: bool,
522+
pub pr_review: bool,
534523

535524
/// Set to `true` to prevent Pull Request reviews from
536525
/// approving or requesting changes.

cpp-linter/src/cli/structs.rs

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -218,12 +218,6 @@ pub struct ClangParams {
218218
/// An optional [`FileFilter`] to exclude files only from clang-format analysis.
219219
pub format_filter: Option<FileFilter>,
220220

221-
/// Assert this if preparing a PR review with clang-tidy advice.
222-
pub tidy_review: bool,
223-
224-
/// Assert this if preparing a PR review with clang-format advice.
225-
pub format_review: bool,
226-
227221
/// The root of the repository, used to locate relative file paths in processing.
228222
///
229223
/// A project-specific cache folder is created in this path.
@@ -284,8 +278,6 @@ impl From<&Cli> for ClangParams {
284278
clang_format_command: None,
285279
tidy_filter,
286280
format_filter,
287-
tidy_review: args.feedback_options.tidy_review,
288-
format_review: args.feedback_options.format_review,
289281
repo_root,
290282
}
291283
}
@@ -309,11 +301,8 @@ pub struct FeedbackInput {
309301
/// The clang-format style to show in file annotations.
310302
pub style: String,
311303

312-
/// Whether to post a PR review with clang-tidy suggestions/notes.
313-
pub tidy_review: bool,
314-
315-
/// Whether to post a PR review with clang-format suggestions.
316-
pub format_review: bool,
304+
/// Whether to post a PR review.
305+
pub pr_review: bool,
317306

318307
/// Should PR reviews be commentary?
319308
///
@@ -334,8 +323,7 @@ impl From<&Cli> for FeedbackInput {
334323
step_summary: args.feedback_options.step_summary,
335324
thread_comments: args.feedback_options.thread_comments.clone(),
336325
file_annotations: args.feedback_options.file_annotations,
337-
tidy_review: args.feedback_options.tidy_review,
338-
format_review: args.feedback_options.format_review,
326+
pr_review: args.feedback_options.pr_review,
339327
passive_reviews: args.feedback_options.passive_reviews,
340328
repo_root: args.source_options.repo_root.clone(),
341329
}
@@ -351,8 +339,7 @@ impl Default for FeedbackInput {
351339
step_summary: false,
352340
file_annotations: true,
353341
style: "llvm".to_string(),
354-
tidy_review: false,
355-
format_review: false,
342+
pr_review: false,
356343
passive_reviews: false,
357344
repo_root: PathBuf::from("."),
358345
}

cpp-linter/src/rest_client.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -183,9 +183,7 @@ impl RestClient {
183183
};
184184
self.client.post_thread_comment(options).await?;
185185
}
186-
if self.client.is_pr_event()
187-
&& (feedback_inputs.tidy_review || feedback_inputs.format_review)
188-
{
186+
if self.client.is_pr_event() && feedback_inputs.pr_review {
189187
let summary_only = ["true", "on", "1"].contains(
190188
&env::var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY")
191189
.unwrap_or("false".to_string())

cpp-linter/src/run.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
123123
FileObj::new(file_path)
124124
})
125125
.collect();
126-
if is_pr && (cli.feedback_options.tidy_review || cli.feedback_options.format_review) {
126+
if is_pr && cli.feedback_options.pr_review {
127127
let changed_files = rest_api_client
128128
.get_list_of_changed_files(
129129
&file_filter,
@@ -152,7 +152,7 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
152152
}
153153
rest_api_client.end_log_group("Get list of specified source files");
154154

155-
let mut clang_params = ClangParams::from(&cli);
155+
let clang_params = ClangParams::from(&cli);
156156
// mkdir -p .cpp-linter-cache/
157157
let cache_dir = clang_params.repo_root.join(ClangParams::CACHE_DIR);
158158
std::fs::create_dir_all(&cache_dir)
@@ -163,8 +163,6 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
163163
"# Automatically created by cpp-linter\n*\n",
164164
)
165165
.with_context(|| "Failed to write .cpp-linter-cache/.gitignore file")?;
166-
clang_params.format_review &= is_pr;
167-
clang_params.tidy_review &= is_pr;
168166
let user_inputs = FeedbackInput::from(&cli);
169167
let clang_versions = capture_clang_tools_output(
170168
&arc_files,

cpp-linter/tests/reviews.rs

Lines changed: 6 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,7 @@ const REMAINING_RATE_LIMIT_HEADER: &str = "x-ratelimit-remaining";
2323

2424
struct TestParams {
2525
pub lines_changed_only: LinesChangedOnly,
26-
pub tidy_review: bool,
27-
pub format_review: bool,
26+
pub pr_review: bool,
2827
pub passive_reviews: bool,
2928
pub no_lgtm: bool,
3029
pub force_lgtm: bool,
@@ -42,8 +41,7 @@ impl Default for TestParams {
4241
fn default() -> Self {
4342
Self {
4443
lines_changed_only: LinesChangedOnly::On,
45-
tidy_review: true,
46-
format_review: true,
44+
pr_review: true,
4745
passive_reviews: false,
4846
no_lgtm: true,
4947
force_lgtm: false,
@@ -175,9 +173,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) {
175173
"REQUEST_CHANGES"
176174
};
177175
let tool_summary = {
178-
if !(test_params.format_review || test_params.tidy_review)
179-
|| test_params.lines_changed_only == LinesChangedOnly::On
180-
{
176+
if !test_params.pr_review || test_params.lines_changed_only == LinesChangedOnly::On {
181177
// results can be non-deterministic because different clang-versions output different diagnostic/changes
182178
".*" // match anything
183179
} else if test_params.force_lgtm {
@@ -226,24 +222,22 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) {
226222
format!("-l={}", test_params.lines_changed_only),
227223
format!("--ignore-tidy={}", tidy_ignore),
228224
format!("--ignore-format={}", format_ignore),
229-
format!("--tidy-review={}", test_params.tidy_review),
230-
format!("--format-review={}", test_params.format_review),
225+
format!("--pr-review={}", test_params.pr_review),
231226
format!("--passive-reviews={}", test_params.passive_reviews),
232227
format!("--no-lgtm={}", test_params.no_lgtm),
233228
"-p=build".to_string(),
234229
"-i=build".to_string(),
235230
format!("--repo-root={}", tmp_dir.path().to_str().unwrap()),
236231
];
237232
if test_params.force_lgtm {
238-
if test_params.tidy_review {
233+
if test_params.pr_review {
239234
// only use a check that doesn't trigger concern on test assets
240235
args.push("--tidy-checks=-*,bugprone-infinite-loop".to_string());
241-
}
242-
if test_params.format_review {
243236
// explicitly disable formatting using `DisableFormat: true`
244237
args.push("--style={DisableFormat: true}".to_string());
245238
}
246239
} else {
240+
// use default --tidy-checks value
247241
args.push("--style=file".to_string()); // use .clang-format file
248242
}
249243
match run_main(args).await {
@@ -279,26 +273,6 @@ async fn all_lines() {
279273
.await;
280274
}
281275

282-
#[tokio::test]
283-
async fn all_lines_tidy_only() {
284-
test_review(&TestParams {
285-
lines_changed_only: LinesChangedOnly::Off,
286-
format_review: false,
287-
..Default::default()
288-
})
289-
.await;
290-
}
291-
292-
#[tokio::test]
293-
async fn all_lines_format_only() {
294-
test_review(&TestParams {
295-
lines_changed_only: LinesChangedOnly::Off,
296-
tidy_review: false,
297-
..Default::default()
298-
})
299-
.await;
300-
}
301-
302276
#[tokio::test]
303277
async fn changed_lines() {
304278
test_review(&TestParams::default()).await;

0 commit comments

Comments
 (0)