Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
6 tasks done
MitchellBerend opened this issue Sep 18, 2022 · 4 comments · Fixed by #100
Assignees
Labels
question Further information is requested refactoring

Comments

@MitchellBerend
Copy link
Collaborator

MitchellBerend commented Sep 18, 2022

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.

  • src/infra/err.rs
    /// Print an error message and exit with code 1
    pub fn abort_with(err_msg: &str) -> ! {
    eprintln!(
    r#"Aborting 'tool-sync' with error:
    * {}"#,
    err_msg
    );
    process::exit(1);
    }
  • src/infra/err.rs
    /// Print an error message, suggesting opening an issue and exit with code 1
    pub fn abort_suggest_issue(err_msg: &str) -> ! {
    eprintln!(
    r#"Aborting 'tool-sync' with error:
    * {}
    Please, open an issue in the 'tool-sync' repository and provide as many
    details as possible to diagnose the problem if you want to get help with
    this issue:
    * https://github.com/chshersh/tool-sync/issues/new"#,
    err_msg
    );
    process::exit(1);
    }
  • src/infra/err.rs
    /// Print just the message and exit
    pub fn abort(err_msg: &str) -> ! {
    eprintln!("{}", err_msg);
    process::exit(1);
    }
  • src/sync/prefetch.rs
    fn expected_err_msg(&self, tool_name: &str, msg: &str) {
    let tool = format!("{}", style(tool_name).cyan().bold());
    self.pb.println(format!("{} {} {}", ERROR, tool, msg))
    }
  • src/sync/prefetch.rs
    fn unexpected_err_msg(&self, tool_name: &str, msg: &str) {
    let tool = format!("{}", style(tool_name).cyan().bold());
    let err_msg = format!(
    r#"{emoji} {tool} {msg}
    If you think you see this error by a 'tool-sync' mistake,
    don't hesitate to open an issue:
    * https://github.com/chshersh/tool-sync/issues/new"#,
    emoji = ERROR,
    tool = tool,
    msg = msg,
    );
    self.pb.println(err_msg);
    }
    fn finish(&self) {
    self.pb.finish()
    }
    }
  • src/sync/progress.rs
    pub fn failure(&self, pb: ProgressBar, tool_name: &str, tag: &str, err_msg: String) {
    pb.set_prefix(self.fmt_prefix(FAILURE, tool_name, tag));
    let failure_msg = format!("{}", style(err_msg).red());
    pb.set_message(failure_msg);
    pb.finish();
    }
@chshersh chshersh added question Further information is requested refactoring labels Sep 18, 2022
@chshersh
Copy link
Owner

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

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.

chshersh pushed a commit that referenced this issue Sep 19, 2022
…lements Display (#100)

Resolves #98

This pr converts all functions that accept `&str` to print them out to
stdout/err to accept any type that implements `Display` in its stead.
This allows these functions to not care about the implementation of the
actual print formatting and leaves that up to the implementation of
whatever needs to be printed.

### Additional tasks

- [x] Documentation for changes provided/changed
- [ ] Tests added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested refactoring
Projects
None yet
2 participants