fix: honor deprecated experimental flags instead of ignoring them - #532
fix: honor deprecated experimental flags instead of ignoring them#532not-matthias wants to merge 1 commit into
Conversation
Greptile SummaryThis PR makes the deprecated cycle-estimation and allocation-exclusion options feed their stable counterparts for both
Confidence Score: 4/5The PR is not yet safe to merge because stale deprecated environment settings can override explicit stable CLI disables, and the changed warning must also comply with the repository logging requirement. The independent stable and deprecated fields are combined with OR, so an explicit Files Needing Attention: src/cli/shared.rs, src/cli/experimental.rs
|
| Filename | Overview |
|---|---|
| src/cli/shared.rs | Adds alias resolution and tests, but logical OR allows deprecated environment settings to override explicit stable false values. |
| src/cli/experimental.rs | Updates deprecated-option documentation and warning text, while retaining direct stderr output contrary to the Rust logging rule. |
| src/cli/run/mod.rs | Correctly routes resolved cycle-estimation and allocation-exclusion values into the run orchestrator configuration. |
| src/cli/exec/mod.rs | Correctly routes resolved cycle-estimation and allocation-exclusion values into the exec orchestrator configuration. |
Prompt To Fix All With AI
### Issue 1
src/cli/shared.rs:174-175
**Legacy settings override CLI**
If a deprecated environment variable remains enabled while the user explicitly disables the stable option—for example, `CODSPEED_EXPERIMENTAL_EXCLUDE_ALLOCATIONS=true` with `--exclude-allocations=false`—the logical OR keeps the feature enabled. The same applies to cycle estimation, so a lower-priority legacy setting can override the user's explicit command-line choice.
### Issue 2
src/cli/experimental.rs:92-97
**Warning bypasses tracing**
The revised deprecation warning writes directly to stderr with `eprintln!`. This violates the repository directive to use tracing macros such as `warn!` instead of `println!` or `eprintln!` in Rust, so the repository requirement must be satisfied before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: honor deprecated experimental flags..." | Re-trigger Greptile
| eprintln!( | ||
| " {} {} has no effect: {} is now controlled by {}.", | ||
| " {} {} is deprecated and will be removed in a future release: use {} instead.", | ||
| style(Icon::Warning.to_string()).yellow(), | ||
| style(*flag).bold(), | ||
| feature, | ||
| style(*new_flag).bold(), | ||
| ); |
There was a problem hiding this comment.
The revised deprecation warning writes directly to stderr with eprintln!. This violates the repository directive to use tracing macros such as warn! instead of println! or eprintln! in Rust, so the repository requirement must be satisfied before merging.
Rule Used: Never use println!/eprintln! in Rust; use trac... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/experimental.rs
Line: 92-97
Comment:
**Warning bypasses tracing**
The revised deprecation warning writes directly to stderr with `eprintln!`. This violates the repository directive to use tracing macros such as `warn!` instead of `println!` or `eprintln!` in Rust, so the repository requirement must be satisfied before merging.
**Rule Used:** Never use `println!`/`eprintln!` in Rust; use trac... ([source](https://app.greptile.com/codspeed/-/custom-context?memory=f04f734b-e556-482b-98b9-64c7ce663663))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Merging this PR will not alter performance
|
`--experimental-cycle-estimation` and `--experimental-exclude-allocations` were parsed but deliberately dropped, so anyone still passing `--experimental-exclude-allocations` silently lost allocation exclusion. Both now feed their graduated counterparts and warn that they are deprecated and will be removed in a future release.
1cf5a8b to
e11c9df
Compare
--experimental-cycle-estimationand--experimental-exclude-allocationswere parsed but deliberately dropped. Since--exclude-allocationsdefaults tofalse, anyone still passing the old flag silently lost allocation exclusion.Both now feed their graduated counterparts, and the warning states the truth instead of "has no effect":
Closes COD-3511