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 note on supported Rust versions for rustdoc JSON format. #139

Merged
merged 5 commits into from
Oct 15, 2022

Conversation

obi1kenobi
Copy link
Owner

@obi1kenobi obi1kenobi commented Sep 28, 2022

Also update a few broken links and references in documentation.

Update broken links and references in documentation.
@obi1kenobi obi1kenobi requested a review from epage September 28, 2022 22:38
@obi1kenobi
Copy link
Owner Author

obi1kenobi commented Sep 28, 2022

Concise writing isn't my strongest side. Feel free to edit as you see fit, especially around which Rust/rustdoc versions are supported.

Also, should we set MSRV 1.64 in Cargo.toml?

Copy link

@aDotInTheVoid aDotInTheVoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this fully explains what/why the situation is. Here's another draft that's probably too wordy

cargo-semver-checks relies on rustdoc-json, a currently unstable rustdoc feature that gives us programmatic access to a crates API. Until that feature is stabilized, cargo-semver-checks cannot guarantee suport for all future versions of rust.

However, we aim to support the current stable/beta. This is done by using RUSTC_BOOTSTRAP, a hack to allow stable/beta compilers to use nightly features. Despite this, this doesn't imply support for future stable releases, so when you update rustc/rustdoc, cargo-semver-checks will also need to be updated

We may also support the latest nightly, but that is harder, as the format changes much more frequently (and with less notice) that faking it with stable/beta

The currently supported versions are listed [here](LINK)

README.md Outdated
- **No false positives**: Currently, all queries report constructive proof of semver violations:
there are no false positives. They always list a file name and line number for the baseline item
that could not be found in the new code.
- **There are false negatives**: This tool is a work-in-progress, and cannot check all kinds of
semver violations yet. Just because it doesn't find any semver issues doesn't mean
they don't exist.
- **Support for most recent stable and beta Rust**: Different Rust compiler versions
produce different rustdoc format versions. The most recent `cargo-semver-checks` always

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different Rust compiler versions produce different rustdoc format versions.

Is it reasonable to assume that a user will understand (or needs to understand) what a rustdoc json format version is?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

README.md Outdated
- **No false positives**: Currently, all queries report constructive proof of semver violations:
there are no false positives. They always list a file name and line number for the baseline item
that could not be found in the new code.
- **There are false negatives**: This tool is a work-in-progress, and cannot check all kinds of
semver violations yet. Just because it doesn't find any semver issues doesn't mean
they don't exist.
- **Support for most recent stable and beta Rust**: Different Rust compiler versions
produce different rustdoc format versions. The most recent `cargo-semver-checks` always
supports the most recent Rust stable and beta versions. It *may* also support some nightly

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most recent cargo-semver-checks always supports the most recent Rust stable and beta versions.

I'd more carefully qualify this, as this says that you will maintain the adapter until rustdoc-json is stable. I'd think long and hard before publicly committing to that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on how you are reading that into it because I'm not.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth clarifying to the user that keeping up with stable requires ongoing maintenance from the maintainers of cargo-semver-checks. This is in contrast all rust crates, where once they work on stable will work on stable without ongoing support.

The way it's written makes it sound like the MSRV is the latest rust, and that you will drop support for old stables immediately, which is much more common in the ecosystem.

@obi1kenobi
Copy link
Owner Author

Since there's a fair bit of nuance we're trying to convey here, is it worth taking that bullet point and giving it is own section with a paragraph or two?

@obi1kenobi
Copy link
Owner Author

I'm going to split out the changes to CONTRIBUTING.md and merge them separately since the current links there are just flat out broken, and the unresolved discussion here isn't related to that file at all.

@obi1kenobi
Copy link
Owner Author

This PR is imperfect, but I think it's better than the status-quo. For example, it much more clearly states that false-positives are bugs and false-negatives are missing features rather than bugs. Having that section in the FAQ would probably have saved at least one user a bunch of work and confusion: #148

@aDotInTheVoid and @epage, I'm not sure if my rewrite fully addressed your concerns, though I did my best. If you get a chance to take a look post-merge and you still have concerns, please add comments and I'd be happy to address them in a follow-up PR.

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.

3 participants