Add ability to specify features in CLI

[staniewzki]

Resolves #321.
Resolves #181.

[tonowak]

🔥 🎉 🚀

[obi1kenobi]

Neat! Excited to see this ship!

[obi1kenobi]

Another two things to consider:

• What happens if a new feature is added in "current" but the same feature doesn't exist in "baseline"? Attempting to generate baseline rustdoc with that feature enabled would fail, since the feature isn't defined there.
• Is there a possible and easy tweak of the CLI that would let us support the use case in Feature compatibility verification #412?

For both of these, please don't implement anything yet and let's just discuss possible options and designs first. It'll be harder to think of good solutions if you've already gone ahead and implemented a particular approach.

With that in mind, what do you think?

[staniewzki]

If a feature doesn't exist in "baseline", it could just be ignored. This seems quite easy to implement.

For the mentioned use case, maybe we could add another flag, that adds features to the "current" version, something like --feature-current? I think it makes more sense this way, as the ususal use case is to test the crates on the same feature set. Also, you never want "baseline" feature set to be a superset of "current" feature set, right?

[obi1kenobi]

If a feature doesn't exist in "baseline", it could just be ignored. This seems quite easy to implement.

This seems reasonable to me, though we should probably print a warning message about it to make sure the user is aware.

For the mentioned use case, maybe we could add another flag, that adds features to the "current" version, something like --feature-current? I think it makes more sense this way, as the ususal use case is to test the crates on the same feature set. Also, you never want "baseline" feature set to be a superset of "current" feature set, right?

"Never" is a very strong word, I wouldn't say never. For example, maybe a library had an optional feature in v1 that is included by default in v1.1, and wants to check that the API is unchanged between v1+feature and v1.1 without the feature.

If we're adding a flag to affect the features in only one of current/baseline, we should add the symmetrical flag for the other one too.

A few other, smaller things:

• cargo uses the plural "features" for its feature-related flags (e.g. in cargo add), so for consistency let's use features as well and make it work the same way as in cargo add. Since we're planning on merging into cargo, it's important that we minimize friction like this.
• Our other flags that are split between current/baseline use the word "current" or "baseline" to start, so let's call the flags --baseline-features and --current-features instead of using "current" or "baseline" as a suffix.
• For the feature-related flags that affect both current and baseline, let's make sure to explicitly document this fact in their help text.

[staniewzki]

@obi1kenobi how do I deal with clippy::too_many_argument lint on generate_versioned_crate function? I know that having that many arguments is not ideal, but I don't think that creating a struct to pack all the parameters there makes much more sense.

[obi1kenobi]

@obi1kenobi how do I deal with clippy::too_many_argument lint on generate_versioned_crate function? I know that having that many arguments is not ideal, but I don't think that creating a struct to pack all the parameters there makes much more sense.

Making a single struct to pack everything doesn't make sense, but perhaps we can group the arguments somehow? For example, consider whether any arguments are always used together with each other: if the function takes an a and b, and then whenever it passes a somewhere it also passes b, then maybe a and b should be combined.

Then see if it makes sense to move one inside the other and if so, in which direction. Or if not, see if it makes sense to make a struct that holds both of them.

This is a skill that takes practice to develop, so I don't want to do you the disservice of just giving you the answer and robbing you of an excellent opportunity to practice the skill in a safe space.

[obi1kenobi]

I love the thought and care you've put into testing this thoroughly, it's awesome to see! It gives me great confidence in the code even though we're going to make a significant change to the cargo-semver-checks behavior 💯

I have one major concern with respect to how we're doing hashing for caching, and otherwise just maintainability / internal implementation design questions and suggestions. I have confidence that the feature works overall, but would like to also make it as maintainable and easy to understand as possible, for the sake of every subsequent piece of functionality we're going to add near this code.

[obi1kenobi]

Can you clarify how this is different from the default behavior? What does adding this flag change?

[staniewzki]

Should I just explain it briefly there? Or list the feature names that aren't enabled by default?

[obi1kenobi]

This is the text that users see when they run --help, right?

Probably both explain it briefly and describe how it's different than not providing the flag.

Try to answer both "what does this do?" and "when would I use it?"

[obi1kenobi]

Ignore the nightly failure for now, there's a new rustdoc format version (v25) and I'm waiting on rustdoc-types to release a corresponding new version before I can release the matching new adapter version for it.

[staniewzki]

I really like the changes I made thanks to the review!

[obi1kenobi]

The code looks in pretty good shape, and I'm excited to merge and release this soon!

This PR can also be a good learning opportunity: 90% if not 100% of the suggestions I made here were things you could have caught too by carefully re-reading your code in the PR review interface. They were things like typos, user-facing text that could be misinterpreted, etc. I'm not in any way special at being able to catch them — but every time I catch them, we have to go through another fix-review loop which due to time zones can take 24h or more. Then you get frustrated that we haven't merged this after a month of work, and I'm frustrated that I'm reading 800 lines of code yet another time. It ends up being a lose-lose for both of us.

One of the most important skills you'll need in industry is the ability to structure your work so that it's easy to merge and meets the quality bar on the first review so that reviewers can just hit the "merge" button after one read. In retrospect, you can probably see that this PR was too large of a slice of work to do at once, and could have been split into "internal engine improvements" (3-4 PRs) + plugging in the CLI and adding end-to-end tests (1 more PR) instead. Even though this is more PRs, you could develop them in parallel and because they are smaller, they'll merge faster. Time-to-merge is exponential in PR size!

If you'd like to chat in more depth about how to structure PRs for more efficiency for both you and the code reviewer, I'd be happy to talk. Ping me on Zulip and we can set up a time.

[obi1kenobi]

I added some suggestions for the CLI. Everything else looks good, let's get this one home and release it!

[obi1kenobi]

I merged main to make nightly start passing again.

[obi1kenobi]

Awesome job 🎉

In the next PR, could you add a few examples of the new functionality to the README? That way they'll show up on crates.io in the next version and I'll also use them for the release notes and for the release thread on Twitter / Mastodon.