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

Changed all baseline rustdoc generations to use placeholder manifest #341

Merged

Conversation

tonowak
Copy link
Collaborator

@tonowak tonowak commented Feb 3, 2023

This PR is a continuation of #340.

This PR changes the way baseline rustdoc of PathBaseline is being generated -- similarly to RegistryBaseline, a placeholder project is created (with a dependency to the true project) on which the rustdoc is generated.

I was worried about two possible places which could have bugs, but after writing this PR, it turned out to not be a problem:

  • the dependency is given an absolute path, instead of a relative path, so the target directory in which the placeholder project is placed doesn't matter,
  • getting all features from a manifest (instead of the registry) turned out to be extremely similar to getting all features from registry. I've split the feature functions into multiple smaller functions, which shows similarities for easier comparison and lesser chance of making a bug.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

I like the direction. Just needs some polishing for maintainability.

I'd also feel a lot more comfortable with these changes if we had automated tests over them that use path and gitrev arguments in the CLI. Consider adding a new job that looks to catch one of the known (but not yanked) semver violations by path and by gitrev, and ensures the output shows the semver issue was found?

src/baseline.rs Show resolved Hide resolved
src/baseline.rs Outdated Show resolved Hide resolved
src/baseline.rs Show resolved Hide resolved
src/baseline.rs Show resolved Hide resolved
src/baseline.rs Outdated Show resolved Hide resolved
tonowak and others added 3 commits February 3, 2023 18:36
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
@tonowak
Copy link
Collaborator Author

tonowak commented Feb 3, 2023

Consider adding a new job that looks to catch one of the known (but not yanked) semver violations by path and by gitrev, and ensures the output shows the semver issue was found?

Do you have some specific crates / versions in mind?

@obi1kenobi
Copy link
Owner

Do you have some specific crates / versions in mind?

I know the affected version for clap is still up, it's a solid candidate.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looks good so far. The tests are the biggest remaining thing, since I believe our entire test suite currently uses registry-based baselines.

I suggested a couple of tiny incremental improvements as well.

src/baseline.rs Outdated Show resolved Hide resolved
src/baseline.rs Outdated Show resolved Hide resolved
src/baseline.rs Outdated Show resolved Hide resolved
@tonowak tonowak requested a review from epage as a code owner February 4, 2023 14:22
@tonowak
Copy link
Collaborator Author

tonowak commented Feb 4, 2023

I know the affected version for clap is still up, it's a solid candidate.

I'm not entirely sure whether I understood what you mean, but I've added a test in the CI. And also I've found a bug in the previous CI :)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@tonowak
Copy link
Collaborator Author

tonowak commented Feb 4, 2023

This PR also fixes #344.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Awesome job!

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