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

Refatoring of all functions that print to stdout/err so they accept a generic type that implements the Display trait #98

Closed MitchellBerend closed 2 years ago

MitchellBerend commented 2 years ago

The following locations are potential candidates. These should not have a big impact on the rest of the code base, but they allow all the errors and other enums to have the same implementation strategy.

I would love some feedback on this.

chshersh commented 2 years ago

@MitchellBerend Thanks a lot for collecting all the usages of errors!

I marked this issue as blocked by #88 as the Display refactoring needs to happen first. Using Display instead of custom display() is definitely a welcome change 👏🏻

For this particular issue, it's a bit hard to evaluate whether it'll simplify the code or make it less maintainable. In theory, specifying the err_msg argument as some polymorphic type <Msg: Display> could open a road to future refactoring where we use some specialised trait for errors. E.g. std::error::Error or something from eyre.

But in any case, if refactoring proposed by this issue makes function signatures more generic and reduces usages of to_string() and display() on call sites, I'm happy to apply this refactoring 🆗

MitchellBerend commented 2 years ago

std::error::Error does not have the Display trait implemented and rust requires either the trait or the struct/enum to be defined in the crate itself, so I'm not sure if we can use that with the proposed change. Im not sure how eyre gets around this though.

MitchellBerend commented 2 years ago

Should we add a blocked label btw? That seems like a good idea so others don't waste time on issues that can't actually be solved yet.

chshersh commented 2 years ago

The blocked label sounds like a good idea! I've created it and added it to a blocked issue.

However, this issue is no longer blocked as #88 actually was already done by @zixuanzhang-x 😅

There's no way to specify what issue is blocking inside the label text. So I propose to put the "Blocked by #NUMBER" string in the beginning of every issue that is blocked by something else.