search

rust-secure-code / cargo-supply-chain

Gather author, contributor and publisher data on crates in your dependency graph.
Apache License 2.0
313 stars 18 forks source link

Add `--no-dev` option #93

Closed smoelius closed 1 year ago

smoelius commented 1 year ago

Resolves #67

Nits are welcome.

Shnatsel commented 1 year ago

Thank you for the PR! I am currently traveling, so I am not sure when I will be able to review it.

If it slips through the cracks and I don't review it within a week, please remind me - leaving a comment here should be sufficient.

smoelius commented 1 year ago

Will do. Thank you, @Shnatsel.

smoelius commented 1 year ago

Friendly ping. No rush, though.

Shnatsel commented 1 year ago

Sorry the review took so long, I was preoccupied with other urgent matters.

The approach looks good to me. The only nit is I'd like to have tests for this functionality, since it's not entirely trivial. Specifically I would like to cover the case when the toplevel package has a dev dependency that in turn has runtime dependencies; these should not be included even though they have DependencyKind::Development.

Thanks again for the PR!

smoelius commented 1 year ago

The only nit is I'd like to have tests for this functionality

That's not a nit at all.*

I was actually considering using trycmd for this. Would that be okay?

If so, are there any particular packages you would like used for the tests?

EDIT: * As in it's a perfectly legitimate request, I mean.

Shnatsel commented 1 year ago

trycmd has a rather sprawling supply chain, and I'm trying to eat my own dog food here and be conscious about supply chain sprawl. It would add 35 publishers to the supply chain in the default configuration, or 19 without the default features.

Would insta suffice instead? It's a popular snapshot testing tool with a much smaller supply chain footprint, and would only add 4 publishers.

I don't really have a preference on the packages to use for tests.

smoelius commented 1 year ago

Would insta suffice instead?

I haven't used insta before, but I suspect I can figure it out.

I'll start on this very soon.

smoelius commented 1 year ago

How is this?

I tried to make the ui test something that is extensible. The no_dev test is then implemented on top of that.

The similar-asserts dependency isn't strictly needed. It just makes the output look nicer when there's a difference.

Hopefully, adding tempfile was okay.

smoelius commented 1 year ago

Is this better?

Shnatsel commented 1 year ago

Yeah, that looks great! Merging. Thanks again!

Shnatsel commented 1 year ago

I'll make a stable release soon, too.

smoelius commented 1 year ago

Thanks, @Shnatsel. I will use this!

Shnatsel commented 1 year ago

I've published v0.3.3 with these changes. Thanks again!