[#33] Better end-of-sync output #80
Conversation
chshersh
added
enhancement
src/model/release.rs
Outdated
| impl From<&Asset> for Asset { | ||
| fn from(other: &Asset) -> Self { | ||
| Asset { | ||
| id: other.id, | ||
| name: other.name.clone(), | ||
| size: other.size, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't know what is the best way to convert from &A to A so found usages of the From trait.
I was also thinking about ToOwned but to me ToOwned looked like it should be used for different types (e.g. &str / String, Path / PathBuf)
There was a problem hiding this comment.
How I understand to_owned is that it is essentially a superset or clone, I am for putting in the smallest scope so the clone way would be best.
There was a problem hiding this comment.
Thanks for the suggestion! Wasn't sure Clone could work here. It simplified code indeed 😌
src/sync/progress.rs
Outdated
| impl SyncProgress { | ||
| /// Creates new `SyncProgress` from a list of tools. | ||
| /// !!! The given `Vec` must be non-empty !!! | ||
| pub fn new(tools: Vec<String>, tags: Vec<String>) -> SyncProgress { | ||
| pub fn new(tool_assets: Vec<ToolPair>) -> SyncProgress { |
There was a problem hiding this comment.
Oops, this should be tool_pairs instead of tool_assets
| } | ||
| } | ||
|
|
||
| eprintln!("{} Successfully installed {} tools!", DONE, installed_tools); |
There was a problem hiding this comment.
Probably can also say where exactly they were installed 🤔
Sometimes, when locally testing with different configs, I don't remember all the directories 🥴
src/sync/prefetch.rs
Outdated
| let estimated_download_size: u64 = tool_assets.iter().map(|ta| ta.asset.size).sum(); | ||
| let size = HumanBytes(estimated_download_size); | ||
| eprintln!( | ||
| "{emoji} Estimated download size: {size}", | ||
| emoji = PACKAGE, | ||
| size = size | ||
| ); |
There was a problem hiding this comment.
Maybe we should include that this is the size of the sum of all tools. This was not clear to me immediately so I had to look that up in the code.
|
Im happy that the structure of the fetching and actual downloading is the way it is in this change. It means actual decisions can be made based on the status of tools in the configuration. This might also enable a prediction about how many of the configured tools can actually be downloaded since there is a rate limit of 60 requests per hour. I think the hard limit we run into right now is 30 tools total. There is 1 api call to find the id of the tag and then 1 call to actually download it. |
Indeed, that's the plan 🙂
If you use a GitHub token, the limit is actually 5000 (and not 60). I believe we have some capacity until people need more than 2500 tools 👍🏻 |
|
Turns out my |
Resolves #33
chshersh
force-pushed
the
chshersh/33-Better-end-of-sync-output
branch
from
d2c31ed
to
ffc96c7
Compare
|
Here is the final GIF after applying suggestions in this PR:
|
Resolves #33
This PR changes the fetching algorithm significantly in an attempt to improve the final output. Specifically,
A single GIF worth a thousand words: