Skip to content

refactor(api): replace multi-bool params with descriptive enums [BREAKING]#119

Open
virtualritz wants to merge 1 commit into
masterfrom
api-bool-params-to-enums
Open

refactor(api): replace multi-bool params with descriptive enums [BREAKING]#119
virtualritz wants to merge 1 commit into
masterfrom
api-bool-params-to-enums

Conversation

@virtualritz

@virtualritz virtualritz commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Addresses the blueprint vet's fn_params_excessive_bools/Rust API Guidelines finding: replace adjacent bool parameters with descriptive enums so call sites are self-documenting.

BREAKING (targets 0.5.0). Each new enum implements From<bool> so migration stays ergonomic (x.into() at call sites that hold a bool).

Changes

Function Old params New enums
aegp set_dynamic_stream_flag undoable: bool, enabled: bool Undoable, FlagValue
aegp select_item select: bool, deselect_others: bool ItemSelection, DeselectOthers
pf/premiere source_track_media_timecode[2] apply_transform: bool, add_start_time_offset: bool ApplyTransform, AddStartTimeOffset

Enums are re-exported from aegp/pf alongside the existing public types, and the higher-level Effect/Item/Stream wrapper methods were updated to match. All internal callers + the supervisor example migrated.

Verification

cargo check --workspace --all-targets --target x86_64-pc-windows-gnu -> clean (exit 0). Vetted on the Windows target; the crate does not build on native Linux.

Honest note

This is the lowest-value item from the cleanup set: each fn had only two, already descriptively-named bools (below clippy's default fn_params_excessive_bools threshold of 3), and enum params are slightly more verbose at call sites that compute a bool.

BREAKING (0.5.0). Replaces adjacent bool pairs with named enums plus
From<bool> for ergonomic migration:
- aegp set_dynamic_stream_flag: Undoable, FlagValue
- aegp select_item: ItemSelection, DeselectOthers
- pf/premiere source_track_media_timecode[2]: ApplyTransform, AddStartTimeOffset
@virtualritz
virtualritz requested a review from AdrianEddy July 13, 2026 17:07
@AdrianEddy

AdrianEddy commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

seems pretty useless to me

Sure, I understand that select_item(x, true, false) is not immediately clear what are the bools for, but this is 2026 and no one is programming without rust-analyzer so seeing the arguments is 1s hover away.

With this change, I now have to locate the correct import for the new enum, and I still have to inspect them anyway to find correct enum variants. Yep, that's a totally useless trade. Exchanges one problem with a new one

@virtualritz

virtualritz commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

That's assuming you run rust-analyer or an IDE.
I have a 16GB laptop when I travel; I can often not afford that nowadays with multiple LLM sessions and 1-2 DCC apps open.

And I often look at code outside an IDE (in a simple editor, like SciTe). I think reasoning about the uselessness/usefullness through the lens of the editing environment is a fallacy.

I would counter this with: why bother at all writing readable code? You an always have an LLM explain the code to you. 😉

I like all the crates I maintain to adhere to official Rust API guidelines. This crate has not seen much love since I didn't use it in any project since a while. But as that is about to change, I want to do better.

This specifically addresses the C-CUSTOM-TYPE rule.

@AdrianEddy

Copy link
Copy Markdown
Collaborator

sure it's more clear when reading without an IDE, but what about writing? You still can't write any of this without knowing the exact signature, so you have to look it up one way or another + you need to add an import which is more work

I also don't think C-CUSTOM-TYPE should apply to everything. The example there is a good use case ( Widget::new(Small, Round) instead of Widget::new(true, false) but select_item(item, true) is perfectly reasonable as bool, although it could also be split to select_item and deselect_item. The second argument might be an enum, since that's not obvious, but to be really clean it could be something like SelectionFlags::{None, DeselectOthers}.
An enum with {Yes, No} is another code smell on its own.

Anyway, that's just my personal opinion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants