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

✨ Add the ability to build an entire workspace or a specific executable #6

Closed
wants to merge 2 commits into from

Conversation

samuelsleight
Copy link
Contributor

This PR adds a BinTestBuilder struct, which can be used to add the --workspace flag to cargo build and/or to build a specific executable with the --bin flag.

This was all fine for my specific use-case, but I'm very open to suggestions or alternative API ideas - if we're happy with this though, we could also use it to handle the RELEASE_BUILD flag too, though I wouldn't necessarily do that as part of this PR

This adds a `BinTestBuilder` struct, which can be used to add the `--workspace` flag to `cargo build` and/or to build a specific executable
@cehteh
Copy link
Owner

cehteh commented Dec 29, 2023

looks good on a first glance. I am out of office, will come back on this next days.

@samuelsleight
Copy link
Contributor Author

All good, no hurry over the holiday period!
I'll be out myself for the first two of weeks of Jan, too, so this might end up waiting until the second half of the month if there's any discussion on it

@samuelsleight
Copy link
Contributor Author

Okay, I squeezed one extra flag into this that I wanted, hopefully that's not too controversial! I should be 100% done with this branch now, as I'm happy with my test workflow where I'm using this

@cehteh
Copy link
Owner

cehteh commented Dec 31, 2023

I created a new 'devel' branch, updated the crate there (msrv, edition, metadata and other bits) and merged this PR. Please test and tell me your opinions.

This will be the base of a 1.1.0-devel release I can do sometime soon, then your rust code using bintest can depend on that.

I did a small change: the builder is now available over the BinTest::with() Api, thus only BinTest has to be imported by a user.

In future we can add more flags to the builder.

Another observation: we could now have a small workspace in this crate creating a binary which is used to test this crate itself :)

@cehteh cehteh closed this Dec 31, 2023
@samuelsleight
Copy link
Contributor Author

Cheers, will check out that branch now and give it a test! The small change certainly seems nicer, keeping the needed imports minimal 👍

@samuelsleight
Copy link
Contributor Author

samuelsleight commented Dec 31, 2023

@cehteh Yep, that's working nicely for me, and I definitely prefer the new API - I'm not 100% sure the name with() is perfect, but I can't think of a better name for it so I'm pretty sure it's fine as-is.

This PR shows the change (and my usecase) if you're curious. (...though something unrelated appears to have broken the CI)

Another observation: we could now have a small workspace in this crate creating a binary which is used to test this crate itself :)

Also I just spotted this line - that seems like a very nifty idea!

@cehteh
Copy link
Owner

cehteh commented Dec 31, 2023

@cehteh Yep, that's working nicely for me, and I definitely prefer the new API - I'm not 100% sure the name with() is perfect, but I can't think of a better name for it so I'm pretty sure it's fine as-is.

Same here, i am open for better name suggestions. I use this 'hidden builder pattern' or how its named frequently but i havent settled on good names yet, sometimes I use 'Foo::build().with_option().with_another_option().finish()' but i am not satisfied with that either.

@samuelsleight
Copy link
Contributor Author

Maybe with is actually alright if the builder methods are sensibly named - then it reads like a Bintest "with" a list of options, which seems decent enough

@samuelsleight
Copy link
Contributor Author

@cehteh What's your plan for a new published release? I've been pointing at the devel for a while now and it's been working perfectly for me after a small change in my own code to lazily run it once instead of running it in parallel which may be worth recommending - I'd now be happy to point this at a crates.io release if/when that's available!

@cehteh
Copy link
Owner

cehteh commented Feb 14, 2024

@cehteh What's your plan for a new published release? I've been pointing at the devel for a while now and it's been working perfectly for me after a small change in my own code to lazily run it once instead of running it in parallel which may be worth recommending - I'd now be happy to point this at a crates.io release if/when that's available!

When you like a stable release ASAP then I can do it. In some older codebase I use lazy_static! similar to how you using the newer Lazy. Which makes me think:

  • This should be documented at least
  • Perhaps the Lazy method should become part of bintest
    • either as breaking API change making BinTest a singleton
    • or by some optional API

So when you don't need it ASAP give me a few day and/or comment on the Lazy idea

@samuelsleight
Copy link
Contributor Author

When you like a stable release ASAP then I can do it. In some older codebase I use lazy_static! similar to how you using the newer Lazy. Which makes me think:

* This should be documented at least

* Perhaps the Lazy method should become part of bintest
  
  * either as breaking API change making BinTest a singleton
  * or by some optional API

So when you don't need it ASAP give me a few day and/or comment on the Lazy idea

Yeah, fair enough - there's no hurry at all really, this is just a personal project so theres's no reall ASAP, it would just be nice if I could remove as many of my git dependencies as I can at some point.

I can file an issue recommending the Lazy pattern, and we can discuss there at some point whether it should be part of the library or documented as a recommendation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants