Refactor of baseline.rs for easier next additive changes #340
Conversation
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
|
Should be ready to merge. |
src/baseline.rs
Outdated
|
|
||
| // We assume that the generated rustdoc is untouched. | ||
| // Users should run cargo-clean if they experience any anomalies. | ||
| if cache_dir.exists() { |
There was a problem hiding this comment.
We need to check for existence of the JSON file, not the directory.
There was a problem hiding this comment.
From what I remember, I've changed the code a bit so that the cached rustdoc is in the format cache/registry-xxx-yyy/rustdoc.json instead of cache/registry-xxx-yyy.json, with the reasoning that the code would be a bit easier to understand after the changes I've made (because of the let (x, y) = z), and I won't deny it -- this change is mostly stylistic. I think the code you've pointed out after my changes is correct -- can you confirm?
There was a problem hiding this comment.
Oh, I see that @obi1kenobi has also commented this code. I'll look into it in detail.
There was a problem hiding this comment.
I'll revert to how it was before.
There was a problem hiding this comment.
I don't think the directory-per-file layout is desirable, and I since we already have something that we think works and that we want to test in the real world, I think we probably don't want to change the behavior here and now.
I'm in favor of reverting.
There was a problem hiding this comment.
42f0fb9 reverts it. Thanks for the help.
There was a problem hiding this comment.
Your PR description said "no functionality was changed" which is why I commented :)
Right, that totally was my mistake, those kind of changes should be in another PR or not present at all. I'm still learning :)
There was a problem hiding this comment.
we don't have any automated tests for this
By a happy accident, we actually do — and they caught a minor bug:
/~https://github.com/obi1kenobi/cargo-semver-checks/actions/runs/4079374830/jobs/7030684022
That test runs effectively the same command over the same crate in two different ways (since they were broken due to separate reasons), and it happens to run back to back in the same runner. So we can see that the caching worked (yay! 🙌) but we can also see that when the cache gets hit, the (baseline) printout is just inexplicably gone (confusing 🤨).
Let's add a user-facing message there that indicates the cache hit. Suggestion: Parsing libp2p-core v0.37.0 (baseline, cached)
There was a problem hiding this comment.
@thomaseizinger you might find the subject crate in the test familiar 😁
There was a problem hiding this comment.
Let's add a user-facing message there that indicates the cache hit. Suggestion:
Parsing libp2p-core v0.37.0 (baseline, cached)
Done in 54e8b67.
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
| @@ -201,76 +389,6 @@ impl RegistryBaseline { | |||
| } | |||
| } | |||
|
|
|||
| /// To get the rustdoc of the baseline, we first create a placeholder project somewhere | |||
| /// with the baseline as a dependency, and run `cargo rustdoc` on it. | |||
| fn create_rustdoc_manifest_for_crate_version( | |||
There was a problem hiding this comment.
Are we just moving this fn around? If so, we could reduce the diff of this PR and make review easier.
There was a problem hiding this comment.
Oh, what do you mean? Is there a way to tell GitHub's diff that some code has moved? Or do you propose to not move this function?
There was a problem hiding this comment.
I would not move it unless strictly necessary if the PR also contains functional changes.
There was a problem hiding this comment.
I see. Since this PR is pretty much reviewed, I think I prefer to leave it as it is currently and in my next PRs I'll try to do it like you suggest (splitting the PRs into functional changes and just moving code around).
|
It's the middle of the night for @tonowak right now, so I'll publish |
|
I just realized -- @thomaseizinger did you need me to generate prebuilt binaries? I published the pre-release version manually to avoid spamming everyone watching the repo, but that also means no prebuilt binaries since the GitHub workflow didn't run. |
tonowak
requested review from
obi1kenobi
and removed request for
thomaseizinger
After this PR, I plan to generate the current&baseline rustdoc from a local copy with the same method as the registry baseline. It will allow specifying features and it won't touch the project's
Cargo.lock.This PR is mostly moving functions and splitting functions into multiple smaller ones -- hopefully no existing logic has been changed. It prepares the code for the new current&baseline rustdoc generation (some new functions are created, but not implemented).