chshersh / tool-sync

🧰 Download pre-built binaries of all your favourite tools with a single command
https://crates.io/crates/tool-sync
Mozilla Public License 2.0
72 stars 17 forks source link

Ensure 'SyncProgress::new()' doesn't take empty Vector #108

Closed chshersh closed 1 year ago

chshersh commented 2 years ago

The SyncProgress::new() method accepts a vector and requires it to be non-empty:

https://github.com/chshersh/tool-sync/blob/df7d4768c9cff2a91616bd9686bc928a52fc8c10/src/sync/progress.rs#L24-L27

It's created here:

https://github.com/chshersh/tool-sync/blob/df7d4768c9cff2a91616bd9686bc928a52fc8c10/src/sync.rs#L72

However, the tool_pairs list might be empty after the prefetch phase 💥

In this case, let's handle this separately and output a message that no tools are required to sync with possible reasons: empty configuration or errors in other tools.

Additional context

There's the nonempty crate that provides a non-empty structure. We could use it but it's an extra dependency and for now, we only need it in one place. So let's just ensure the invariant in one single place for now.

zixuan-x commented 2 years ago

We want to handle this error in sync_from_config_no_check() right? The empty configuration error is already handled in sync_from_config(), so I guess we just need to handle whatever error may have happened in the prefetch phase?

chshersh commented 2 years ago

@zixuanzhang-x You're correct 😌

sync_from_config() checks if user specified any tools at all. But even if they specified, we still might get an empty list of tools to download after this line:

https://github.com/chshersh/tool-sync/blob/df7d4768c9cff2a91616bd9686bc928a52fc8c10/src/sync.rs#L62

So I would add a check immediately after the previous line and either stop immediately by outputting a message if the list is empty or by continuing with the existing flow.