Repository navigation
Fades: Create refuses a shape that is not a string over a selection - #1053
Merged
Merged
Conversation
edit.fades_create over tracks and a range read a null, boolean or number shape as absent and made Equal Power fades. It now returns an error that lists the accepted shapes, as the clip form already did, and leaves the session and the undo history unchanged.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Parse and test fade shapes crates/engine/src/cmd/edit.rs |
The command defaults an omitted shape to EqualPower and rejects supplied values that are not recognized strings. The test checks invalid values and confirms that the session and undo history remain unchanged. |
Priority:
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Bug fix
Possibly related PRs
- storytold/soundcraft#976: This PR also changes
shapehandling inedit.fades_createand adds related shape parameters.
Suggested reviewers: echelon, reoniazx
Merge Risk: 
a41f2
-
The fade-shape validation change is ready to merge.
Architecture risk: 
a41f2
-
The change affects 1 system.
Changed systems:
cratesArchitecture concerns
No architecture-level concerns identified.Review details
-
Systems and components
- observed — crates (service) was modified; 1 changed file maps to changed impact.
Before / after behavior
- observed — Modified behavior in crates/engine/src/cmd/edit.rs:
edit.fades_createnow parsesshapefrom the JSON value directly. Omission defaults toEqualPower; supplied values must be recognized fade-shape strings or the command returns an error listing the accepted shapes. - observed — Modified behavior in crates/engine/src/cmd/edit.rs: Removed the targeted-fade check that separately rejected non-string
shapevalues; shape parsing earlier in the function now handles supplied values. - observed — Modified behavior in crates/engine/src/cmd/edit.rs: Added a test asserting that range fades reject string, null, boolean, and numeric invalid shapes without changing the session or undo history.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main change: range fade creation now rejects non-string shape values. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On
main,edit.fades_createover tracks and a range reads ashapeofnull,trueor1as absent and makes Equal Power fades. The clip form (clip,side,length) already refuses a shape that is not a string.shapeonly as a fade shape name. Leaving it out still gives Equal Power. Any other value, an unknown name included, returns an error that lists the accepted shapes (linear|equal power|s-curve|exponential|logarithmic), and the session is unchanged.New test
range_fades_refuse_a_shape_that_is_not_a_shape_name(engine):"bad",null,trueand1are each refused over a range, and the session and undo history stay as they were. Onmainit fails onnull.The branch merges cleanly with
mainatee050db. It conflicts with #976 in the first lines offades_create, where #976 addsin_shapeandout_shape; a merge keeps both changes.Checks:
cargo test -p soundcraft-engine --lib cmd::edit: 29 passedcargo fmt --all -- --check: cleancargo xtask cion Windows 11 (rustc 1.99.0,SOUNDCRAFT_NO_SYSTEM_PLUGINS=1), ona41f254merged with Autosave: allow the Unix-only mut, so clippy passes on Windows #1017 and CLI tests: the several-roots test uses plain paths, so it passes on Windows #1018, which fix two failuresmainhas on Windows: 7 steps ok (fmt, clippy, test, doctest, assets, layers, wasm), 1,275 passed, 0 failed, 1 ignoredSummary by CodeRabbit