[#98] changed all formatting functions so they accept a type that implements Display #100

MitchellBerend · 2022-09-19T09:41:03Z

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

Documentation for changes provided/changed
Tests added

…s Display

MitchellBerend · 2022-09-19T09:54:32Z

Im not sure what tests I could add here, I haven't actually verified that the tests that are run and succeed actually cover this refactor fully. There .to_string call in a test is still required for conversion sake.

chshersh

Looks neat! I really like this refactoring 🤤

I have a few questions I'd like to know the answer to before merging. But it already looks great 👏🏻

chshersh · 2022-09-19T10:01:19Z

src/infra/err.rs

 use std::process;

 /// Print an error message and exit with code 1
-pub fn abort_with(err_msg: &str) -> ! {
+/// This function can take in any type that implements the [`Display`] trait
+pub fn abort_with<Message: Display>(err_msg: &Message) -> ! {


I'm curious if we actually need & now.

Previously I used &str so I can specify static strings without String::from() or similar. But I imagine that Display is implemented for both the value itself and the reference type.

It also makes more sense to remove & in some semantic meaning. We use errors only once — to report them. So they shouldn't be used afterwards and can be safely moved inside error reporting functions.

So I expect that removing & from all type signatures near the Message and from all use sites will simplify the code further.

For the abort errors this might not make sense actually since the program exits right after.

I'm not 100% sure what the move semantics are here, I know println automatically take references to whatever is passed in but the function still moves the error out of the overarching scope. So in my mind the functions that don't exit the program should always take a reference to the error so it does not get dropped after printing it to stdout/err.

Edit: Removing the references in the signature here actually does not prevent compilation, I guess the compiler is smart enough to see that the objects always get dropped upon exiting the program.

chshersh · 2022-09-19T10:01:46Z

src/infra/err.rs

@@ -13,7 +15,8 @@ pub fn abort_with(err_msg: &str) -> ! {
 }

 /// Print an error message, suggesting opening an issue and exit with code 1
-pub fn abort_suggest_issue(err_msg: &str) -> ! {
+/// This function can take in any type that implements the [`Display`] trait
+pub fn abort_suggest_issue<Message: Display + ?Sized>(err_msg: &Message) -> ! {


Why do we need ?Sized here and not in other functions? 🤔

There is an error that passes the message as a &str, the compiler complains about this. I could try and box that slice but that means the slice lives on the heap even though its actually already known at compile time.

Maybe the Box is actually a good thing, since all the output functions always just take a Display trait bound. Having a consistent api does sound nice.

chshersh · 2022-09-19T10:02:24Z

src/sync/prefetch.rs

        let tool = format!("{}", style(tool_name).cyan().bold());
        self.pb.println(format!("{} {} {}", ERROR, tool, msg))
    }

-    fn unexpected_err_msg(&self, tool_name: &str, msg: &str) {
+    /// This method can take in any type that implements the [`Display`] trait
+    fn unexpected_err_msg<Message: Display>(&self, tool_name: &str, msg: &Message) {


I really like how only type-signatures changed but not the implementation of functions and the call sites became simpler as well! 🚀

Yes this is a perfect application of polymorphism, it does complicate the code a bit for people that aren't used to generic programming, but this cleans up the code a lot in my opinion.

Indeed, I like the simplification in this particular case 🙂

In the end, the generic function is written only once and you need to understand it once. But it's used many times and it can save a bit of boilerplate each time.

…d Boxed the offending &str

… exit the program

chshersh

Beautiful! Only one last comment 🙂

chshersh · 2022-09-19T11:27:10Z

src/config/schema.rs

@@ -64,14 +64,14 @@ impl Config {
        let expanded_store_directory = shellexpand::full(&self.store_directory);

        let store_directory = match expanded_store_directory {
-            Err(e) => err::abort_with(&e.to_string()),
+            Err(e) => err::abort_with(e),


This is perfect 👨🏻‍🍳 🤌

chshersh · 2022-09-19T11:30:58Z

src/lib.rs

@@ -37,7 +37,7 @@ fn resolve_config_path(config_path: Option<PathBuf>) -> PathBuf {
                path
            }
            None => {
-                err::abort_suggest_issue("Unable to find $HOME directory");
+                err::abort_suggest_issue(Box::new("Unable to find $HOME directory"));


I tried removing Box::new from here and it compiles fine. Could we remove it then?

Suggested change

err::abort_suggest_issue(Box::new("Unable to find $HOME directory"));

err::abort_suggest_issue("Unable to find $HOME directory");

Huh weird, I guess maybe the reference change made this possible.

Edit: Ah yes I guess it would make sense that the size is not known since its behind a reference. I thought since its a static string compiler magic would just put it on the stack automatically.

chshersh

Amazing ✨

changed all formatting functions so they accept a type that implement…

2fb00a5

…s Display

MitchellBerend added help wanted Extra attention is needed output Fancy (and not so) output of the tool refactoring labels Sep 19, 2022

removed .to_string calls

247f69d

MitchellBerend removed the help wanted Extra attention is needed label Sep 19, 2022

added a small description about the display trait in each function

ec1fe13

MitchellBerend marked this pull request as ready for review September 19, 2022 09:51

MitchellBerend requested a review from chshersh as a code owner September 19, 2022 09:51

chshersh assigned MitchellBerend Sep 19, 2022

chshersh reviewed Sep 19, 2022

View reviewed changes

MitchellBerend added 3 commits September 19, 2022 12:11

changed abort_suggest_issue to not look at the Sized trait and instea…

71fcac3

…d Boxed the offending &str

removed reference from type signature since the functions here always…

ae5f358

… exit the program

removed useless references

1d1d939

chshersh reviewed Sep 19, 2022

View reviewed changes

removed boxed form of str slice

001c0d0

chshersh approved these changes Sep 19, 2022

View reviewed changes

chshersh merged commit 7700be2 into chshersh:main Sep 19, 2022

MitchellBerend deleted the 98-error-display-refactor branch September 19, 2022 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#98] changed all formatting functions so they accept a type that implements Display #100

[#98] changed all formatting functions so they accept a type that implements Display #100

MitchellBerend commented Sep 19, 2022 •

edited

Loading

MitchellBerend commented Sep 19, 2022

chshersh left a comment

chshersh Sep 19, 2022

MitchellBerend Sep 19, 2022 •

edited

Loading

chshersh Sep 19, 2022

MitchellBerend Sep 19, 2022

chshersh Sep 19, 2022

MitchellBerend Sep 19, 2022

chshersh Sep 19, 2022

chshersh left a comment

chshersh Sep 19, 2022

chshersh Sep 19, 2022

MitchellBerend Sep 19, 2022 •

edited

Loading

chshersh left a comment

	err::abort_suggest_issue(Box::new("Unable to find $HOME directory"));
	err::abort_suggest_issue("Unable to find $HOME directory");

[#98] changed all formatting functions so they accept a type that implements Display #100

[#98] changed all formatting functions so they accept a type that implements Display #100

Conversation

MitchellBerend commented Sep 19, 2022 • edited Loading

Additional tasks

MitchellBerend commented Sep 19, 2022

chshersh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchellBerend Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chshersh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchellBerend Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

chshersh left a comment

Choose a reason for hiding this comment

MitchellBerend commented Sep 19, 2022 •

edited

Loading

MitchellBerend Sep 19, 2022 •

edited

Loading

MitchellBerend Sep 19, 2022 •

edited

Loading