Revise the API of is_manifest_current. #3701
Conversation
There was a problem hiding this comment.
Seems good to me. There don't seem to be any public users of the function, so this shouldn't be too bad of a breaking bugfix, I think.
| @@ -455,7 +455,15 @@ function test(ctx::Context, pkgs::Vector{PackageSpec}; | |||
| return | |||
| end | |||
|
|
|||
| is_manifest_current(ctx::Context = Context()) = Operations.is_manifest_current(ctx.env) | |||
| is_manifest_current(ctx::Context) = Operations.is_manifest_current(ctx.env) | |||
There was a problem hiding this comment.
It broke support of Julia 1.11 in Quarto (which is fixed by now) 😄 Generally, shouldn't documented functionality in Pkg.API be stable and not be broken in semver-nonbreaking releases? Maybe this should be changed to
| is_manifest_current(ctx::Context) = Operations.is_manifest_current(ctx.env) | |
| is_manifest_current(ctx::Context = Context()) = Operations.is_manifest_current(ctx.env) |
again?
There was a problem hiding this comment.
I don't have a strong opinion about this but if you managed to find a working use case for it I guess it has some value. But I'd suggest opening a new PR rather than adding a change suggestion to a merged one.
There was a problem hiding this comment.
I wanted to discuss these questions and get some feedback before opening a PR. I only included the suggestion to clearly illustrate what change I had in mind 🙂
The API function
is_manifest_currentwas not working as intended and was mostly useless for its intended purpose of verifying in tests that the manifest and project files of a package were synchronized. See#2815 (comment) for details.
This PR removes the zero-argument method of
is_manifest_currentand adds a newis_manifest_current(::AbstractString)method that can be used in tests in, e.g., the form