HostId enum Refactor - #1274
Conversation
+ Added `host_id` sub-module, which contains the `HostId` non-exhaustive enum, and all associated implementations and definitions. + Added internal implementations in `platform::impl_platform_host`. + Added implementation of `TryFrom<HostId> for Host`. * Fixed links broken by moving `host_from_id` and `available_hosts`. * Added additional error kind to `host_from_id`. - Removed all definitions and impls of `HostId` from `platform`. - Removed `__cpal_select_host_name` internal macro. - Removed `default_host` from all `platform_impl` modules.
* Fixed hanging indentation on doc comment. - Removed import of `HostTrait` in `platform_impl` of `wasm-bindgen` target.
|
Ack! Rookie mistake. All green now, but I'm not sure if I'm entirely keen on pushing as-is. I think the |
1rhino2
left a comment
1rhino2
left a comment
There was a problem hiding this comment.
static pass on the HostId refactor (no runtime concerns here).
direction looks right for #1260: a single non_exhaustive HostId with every variant, plus is_supported / is_available / available_hosts(), and TryFrom for Host. that is cleaner than cfg-gated enums for cross-platform user code.
a few things to nail before merge
- AvailableHostsIter::next currently returns hosts where is_supported() is true, but the docs on available_hosts / HostId::available_hosts talk about availability. supported != available (daemon down, etc). looks like the iterator should call is_available(), or the naming/docs need to match whichever filter you want.
- Display writes ascii-lowercase ("wasapi") while FromStr examples/docs show mixed case like "WASAPI" / "ALSA". case-insensitive parse is fine; worth one golden test that Display round-trips through FromStr.
- body notes available_hosts / host_from_id / ALL_HOSTS may be obsolete. if they stay, mark deprecated in the same PR so the migration path is obvious; if they go, UPGRADING.md needs a short note.
- needs a rebase check against current master (this sat a bit).
i can help poke windows HostId::Wasapi / Asio availability after rebase if useful.
|
Valid point that supported isn’t equal to available. But to your last point, in which cases would that be different for WASAPI or ASIO? |
3 participants
Original Issue: #1260
This implements the change to having
HostIdinclude a variant for all possible hosts under any compilation configuration.This also adds an implementation of
TryFrom<HostId> for Host, which seemed the best way to have that. This also keepsavailable_hosts,host_from_id, andALL_HOSTS, although they no longer seem relevant to keep anymore.