Considerations

[CosmicHorrorDev]

(preface: I'm a very picky person when it comes to code quality so don't take any of this advice as an attack)

So it took a while to try and get this all into a coherent set of ideas.

Tooling

• I see that you are already using rustfmt which is great for keeping style consistent!
  • It does look like you use comment_width which seems to be unstable so that requires nightly and beyond that from the docs there seems to be no effect unless warp_comments = true as well which is not the default.
• I would also heavily recommend using clippy since it's great for picking up a lot of small warnings and already picks up a lot of great ones on the current repo like unnecessary casts, and other cases to make behavior clearer.

For both of these, you can even check them in CI as well to ensure that it is followed.

Idiomatic Changes

Proper Error handling

There are a couple of posts that go more in-depth with this and I would definitely give them a read in order if you have time ([1] [2]). Currently, it looks like you're using String and Box<dyn Error> to both handle the same problem which is how to handle functions that could encounter multiple types of errors. I won't go too in-depth with this since the posts I mention above do a great job of that so instead I'd like to touch on the weak parts of both approaches. The gist for the solution would be to create your own error type(s) to handle these cases. This is also where using some third-party crates like thiserror, anyhow, and snafu can save you some work as well.

Downsides from Using String for Errors

1. Opaque

So let's take a look at Config::new(...), if I were using this and I looked at the return type (Result<Config, String>) then I can see on the success it returns Config which makes sense, but on failure, it returns String which from a type-level I don't really know too much about what things the error could represent. This is unlike an enum where there are a set number of variants that can represent the failure case and if you want extra data associated with it then rust's enums are able to handle this still.

2. Matching isn't as Ergonomic on Strings as Enums

So if I wanted to handle the errors separately (like automatically creating an empty config file when missing) then you would often see behavior like

match Config::new(...) {
    "Unable to open config file." => {
         // Fallback to some behavior to check and create a file if missing
    },
    "Unable to read config file." => {
        // Do some other behavior
    },
    _ => panic!(...)  // This shouldn't ever be hit
}

So this run's into several issues. First the match arms are pretty verbose since they are the full strings while matching on portions would be pretty error-prone. Beyond that rust doesn't know as much about your error Strings as it would an enum so if you updated the error message then you would have to remember to change the match as well since rust is blind to what the strings represent. Rust can enforce that the enum variants all make sense for the different arms on the other hand. This kinda runs into matching being exhaustive so if an error was an enum you would have to handle all possible variants while this doesn't really work with Strings since the number of options make exhaustive matching essentially impossible without a catch-all case.

Downsides of Box<dyn Error>

So this is a step in the right direction where you can at least match on the separate kind of errors, but like with Strings, just seeing Box<dyn Error> doesn't give you a great indication of what the errors could be. You can at least match on the different kinds of underlying errors and do behavior there so that let's you leverage the type system, but it still struggles with exhaustive matching in this case as well.

Try to Use More Flexible Parameters

Like anything this should be with good measure, there's no reason to try and make everything as generic as possible, but there are a couple easy ones that it would be good to get in the habit of using.

1. &[T] over &Vec<T> where &[T] allows you to pass in &Vec<T> just fine as well as other kinds of slices that you may get. clippy has more info on this if you run it and follow the link.
2. &str over &String I'll go over string types more in a bit, but &str allows you to pass in &str as well as &Strings while &String will not allow you to pass in &str.

Use str over String when possible

(this post covers this pretty quickly)

This is one of those system programming principles, but rust has different string types for a reason and it seems like there are a lot of places where String is unnecessarily used (such as all the error_messages passed to run_tmux_command).

This of course can't be done everywhere, build_pane_args allocating for a new string with format will prevent you from being able to return a Vec<&str> since the memory would need to be allocated in the function to allow you to use it after returning (same problem you'll run into with C).

Use Path over String

This one doesn't really matter as much here because you're not really using the paths to do anything path specific. But just as general advice it is preferred to use Paths when you're trying to represent a filepath over Strings.

Traits!

So this was technically already touched on, but there's a lot of powerful behavior you can get from implementing traits.

Display

The ones that stand out to me would be Display which will give you .to_string() for free. I think clippy also mentions this one, but normally when someone else sees .to_string() used on a custom type they assume that it's implemented through Display which means that they should be able to do any of the normal Display stuff which isn't the case here.

Error

I already covered this a bit and the posts I linked cover it more too. I'm just mentioning it here to be thorough.

Default

This is a good one to implement to represent what the default value should be for custom types. Pane for example can implement this with just a derive annotation since all the types inside it are implement Default as well. There's some work I might try to get through to simplify the start_directory logic that would benefit from Window and Pane having Default implemented (but that's for a separate, much less verbose, issue).

Clone and Copy

So rust has a pretty cool way to represent heavy vs lightweight structures (I think this also might also apply to how you've been doing references to nearly everything in functions). So if a structure is considered heavy then you can derive Clone on it to indicate that it is able to be copied, but that that copying is a heavy (normally heap-allocated) endeavor. Copy on the other hand indicates a light-weight object that can be freely copied without really having to worry. A lot of the simple primitive data types like integers, floats, bool, etc. implement this. A good example of something that could be Copy would be all the enums currently since they only take up 8 bytes. Implementing one of either of these traits can make dealing with the borrow checker a lot easier.

Opinionate Changes

Now that we're through all that it's time for some more opinionated recommendations.

Using a third-party library for command line arg parsing

structopt and clap are both great options here and I see you left a comment about it in the code, but command line arg parsing is pretty hard to do well and the current implementation doesn't scale well / isn't ideal from a user viewpoint. When I was first going to use the library I ended up just jumping into the code to figure out what commands were possible and how arguments could be passed in. Beyond that, things like rmuxinator [-h|--help|help] didn't return any useful information for determining usage since the error messages are pretty bare. Using something like clap would fix this problem along with having the benefit of being a well tested library and actively developed library. I understand wanting to avoid using third-party libraries, but it also greatly cuts down on the extra work that this program has to do.

Setting up Objects

There are a lot of places where there are functions related only to specific structs so it would be nice to switch them to methods, but eventually you could get run to something like

fn run(config: Config) {
    let session = Session::new(config);
    session.spawn();
}

impl Session {
    fn new(config: Config) -> Self {
        ...
    }

    fn spawn(self) {
        let args = self.build_args();
        run_tmux_command(args);

        for hook in self.hooks {
            hook.spawn();
        }

        for (i, window) in self.windows.iter().enumerate() {
            if i > 0 {
                window.spawn();
            }
            window.attach_panes();
        }
    }
}

I'm not a big fan of OOP normally, but I think there is a lot of information that's tied together and this sets up a clean way of using it.

Excessive Borrowing

A lot of the functions seem to borrow a lot of the parameters as a way to deal with the borrow checker. There's some parts where this is definitely excessive like &usize where it's just as cheap to have the usize be copied and passed in. Other places where this stands out is when something is when a string is borrowed into a function just to have it be cloned internally (build_window_start_directory, build_pane_start_directory, etc.). Cloning outside the function also allows you to pass in a String instead of &String.

Small Nits

And just for the really small things I think having

type StartDirectory = Option<String>;

hides the type more than providing a meaningful representation, since I wouldn't expect the start directory to inherently be an option.

And the last one is that I think it makes more sense for run_tmux_command to return a result that can be expected after the function call. This mostly just comes from how the error_message is passed into the function when it's pretty equally verbose between

let error_message = "Something went wrong";
run_tmux_command(..., error_message);

// Compared to
run_tmux_command(...)
    .expect("Something went wrong");

And that's all I got. Hopefully, you find this helpful in some regards. I'd be more than happy to submit some new issues and PRs to address the above, but it's also be good practice to give it a shot yourself.

[ethagnawl]

Thanks, yet again, @LovecraftianHorror. You've gone far beyond what I was expecting after your initial message and I really can't say thank you enough for your time and feedback. I was planning to write a separate blog or Reddit post about this (and still will), but I've been floored by how welcoming the Rust community has been. I know the community has that reputation, but I've really felt the effects since posting about my little experimental project. I won't drag other communities through the mud, but that has not been my experience elsewhere. (I've had to delete similar posts in other PL community subreddits.) I haven't received any feedback that wasn't constructive and/or encouraging and it's really been a breath of fresh air.

Now that that's out of the way ... !

Tooling

rustfmt

You're totally right about my line width setting. That's a combination of old habits and laziness. I will remove the config file and update my vimrc to be more intelligent about when it sets a colored column.

I'll also add a step to the CI to format incoming PRs. I should note that setting up the GH Action for this project took ~30 seconds courtesy of one of their pre-existing templates. That was my first time using one of the templates, but I have used it on other projects and they really seem to have nailed that offering.

clippy

I will certainly look into adding this to my workflow and potentially CI. It looks like there are a couple of Vim plugins which add editor integration but neither has been touched in a while -- they may just be stable, though.

Idiomatic Changes

Proper Error handling

I totally agree and will think about coming up with some contextual custom error types. It's funny, I've learned many of these lessons in Haskell/Elm but some amount of that intuition gets lost when you're fumbling around in a new language and just trying to get a program to compile and run.

Try to Use More Flexible Parameters

Great point.

Use str over String when possible

I'll need to revisit this and see when/where I can make changes. I also admittedly need to read some more about the distinctions. For anyone else coming across this (and maybe for you?), I found this article informative when I read it a few months ago, but (clearly) I should revisit it.

Use Path over String

Great point. This will become even more important if I decide to copy any of tmuxinator's project directory features (e.g. the application looks for config files in a set of known/custom directories in addition to taking a path to a file as a CLI arg).

Traits!

I will need to dig into this all a bit more and see what could potentially be used where -- in addition to the instances you've mentioned. This is all a useful refresher, though!

Opinionate Changes

Using a third-party library for command line arg parsing

I wholeheartedly agree and, as mentioned, plan on integrating clap. In addition to helping me quickly getting a prototype working, it was instructive to work through parsing CLI args on my own.

Setting up Objects

This does seem like a logical grouping. I'll have a think on it.

Excessive Borrowing

You're absolutely right in that a lot of what you're seeing here was

1.) to satisfy the borrow checker 2.) to help with debugging during development (i.e. Logging values after they'd have been consumed -- this is something I meant to ask about in my post. I think it's something I just need to accept, though.) 3.) it initially felt weird to have a function's signature accept a bunch of arguments of different types/kinds (e.g. scalar vs reference) but that's something else I need to get over

Small Nits

opaque directory

Considering that the starting directory (at each level: session, window and pane) is optional, I'm curious to know what you think a better solution would look like.

run_tmux_command return type

I was actually going back and forth about this during development. (I cannot remember why I thought passing the error in and exiting was a good idea. I'll chalk it up to being a new parent who's chronically sleep deprived.) I think you're right, though, and that returning a value from the function and handling it in run is more intuitive and flexible. Depending on what's contained in the returned value, that approach could also potentially allow for more useful, contextual error messages.

Hopefully, you find this helpful in some regards. I'd be more than happy to submit some new issues and PRs to address the above, but it's also be good practice to give it a shot yourself.

This was all very helpful. You've shared some great suggestions/resources and forced me to think about some decisions and corners of this project that I haven't thought about in a few months. If you're itching to take a pass at any of the above, please feel free. I'm going to plan to do the same, but as mentioned, I don't have too much free time these days.

P.S. Do you have a favorite non-profit or know of anything similar to Clojurists Together for Rust? :smiley_cat:

[CosmicHorrorDev]

Tooling

Rustfmt

I don't mind the line length necessarily (I normally use an 80 or 90 width restriction). I just figured it would be good to know that it doesn't work great for comments currently. My solution for this, if you're interested, is to set up language-specific configurations in ftplugins, and then any project-specific ones that don't follow that get a local vimrc configuration via this plugin. Then prevent the file from getting into version control the _vimrc_local.vim with using a global gitignore. It's not the easiest setup but it works pretty well now that all of it is setup.

Clippy

Conveniently enough I use neovim so maybe I can help out a bit here. I know with ALE that I can get the lints to show up, but clippy itself seems a bit finciky where making changes and then rerunning will sometimes stop showing lints. You might have more luck here with coc and I know rust-analyzer is really good for providing some nice features too even if it's a bit heavy when using it on my laptop.

Opinionated Changes

Using a third-party library for command line arg parsing

Oh, I totally understand that it would be a good exercise. There's been countless times where I've reimplemented data structs or synchronization system just to get a better idea of how they work under the hood.

Excessive Borrowing

I really struggled with the 3rd one coming from C++ where passing in large objects directly into a function is a big no-no most of the time since that's a heavy copy, so it took me a while to get used to rust's different semantics regarding passing values.

opaque directory

This is the point where I definitely meant opinionated changes when I said it. I guess to me there are plenty of other areas where things like Layout is also inherently optional, but it's expressed as an Option<Layout> to represent this. This is all personal preference though so if it did end up being a Path later on then I think an Option<Path> would directly show that it's likely an optional path to the starting directory. This is all up to you though, you're more than welcome to leave it how it is currently and it is used quite a lot at the very least.

... being a new parent ...

Congratulations! :partying_face:

P.S. Do you have a favorite non-profit or know of anything similar to Clojurists Together for Rust? :smiley_cat:

I don't really know of any non-profits centered around rust although I'm some exist. I think really just getting a more mature ecosystem with programs like this one to help draw attention is the best thing at the moment for such a young language. Maybe also trying to support some of the people working on this if anything, I'm not really too sure

Well, I think that goes over everything I wanted to cover. There's more I would like to have figured out primarily like if there is a cleaner way of testing the test_for_tmux command with mocking of some sort, but passing in the "tmux" value seems to be the best method from what I can tell. I guess I'm too used to python where you can get away with some really weird stuff when unit testing. I'll definitely be around to add in some more issues and PRs later on, although that might be pretty sporadic depending on how much work my classes assign.

[ethagnawl]

Donation

That was sort of a two part question. Aside from Rust specific organizations, is there a donation I could make on your behalf as a way of saying thanks for your feedback?

if there is a cleaner way of testing the test_for_tmux command with mocking

This is still one of the open questions from my original post. We do this extensively in tmuxinator integration tests to validate that the expected values are being sent to/from the program and shell/tmux/filesystem/etc. I haven't dug too deeply into the available options or how other projects are doing this in Rust yet, but the options seem to be (in order of preference):

• A Rust library like assert_cmd or rexpect
• A program (Rust C FFI?) which uses strace/ptrace to mock and/or make assertions about incoming/outgoing values
• a Bash script and environment trickery

[CosmicHorrorDev]

Neither of them are specifically for rust, but I think Girls Who Code and freeCodeCamp are both non-profits that are trying to make programming more accessible.

I'd agree with your preference there. I'm going to try and focus on reducing down some of the complexity of run currently, but I'd really be interested in delving into some more involved unit and integration testing.

[ethagnawl]

Girls Who Code

This is a great suggestion. I've just made a donation in your honor. :smiley_cat:

Integration testing

I've taken a first pass at this using assert_cmd and am really happy with how it came together. There's more to do, but I think it's a solid foundation.

Anyways, thanks again!