-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Deprecate LibGit2.owner in favour of repository #20135
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
Conversation
base/deprecated.jl
Outdated
| SharedArray{T,length(dims)}(filename, dims, offset; kwargs...)) | ||
|
|
||
| # LibGit2.owner -> LibGit2.repository for consistency | ||
| eval(Base.LibGit2, :(Base.@deprecate_binding owner repository)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, I think you need @deprecate here rather than @deprecate_binding, as the latter is for types and such. So something like
eval(Base.LibGit2, :(Base.@deprecate owner(x) repository(x)))There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that, apparently @deprecate_binding works just fine. TIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that @deprecate (and presumably @deprecate_binding) both export. This came up in #20104. Perhaps we need a non-exporting @deprecate?
d381cf2 to
12c09ca
Compare
|
was owner previously exported from the libgit2 module? if not, this would cause it to be exported which you probably don't want |
|
It wasn't, I don't think. How can I deprecate it without exporting it? |
|
manually call depwarn. simon has a commit in one of his pr's that adds a |
12c09ca to
c0e079e
Compare
base/deprecated.jl
Outdated
| end | ||
|
|
||
| # LibGit2.owner -> LibGit2.repository for consistency | ||
| eval(Base.LibGit2, :(Base.@deprecate_binding owner repository)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need the deprecate_binding (which will still export) if you define the method below
c0e079e to
a73155a
Compare
base/deprecated.jl
Outdated
| function LibGit2.owner(x::Union{LibGit2.GitIndex, LibGit2.GitTree, LibGit2.GitReference}) | ||
| depwarn("owner(x) should be replaced with repository(x)", :owner) | ||
| LibGit2.repository(x) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other deprecations, I would do
function LibGit2.owner(x)
depwarn("owner(x) is deprecated, use repository(x) instead.", :owner)
LibGit2.repository(x)
endThe point is the wording of the message, but note as an aside that I've left out the specification of the input type. I think it's unnecessary in this case, because existing code shouldn't be affected and new code will get a MethodError from repository for any weird input.
I'm not certain but it might even be better to do it as an eval into LibGit2:
eval(LibGit2, quote
function owner(x)
depwarn("owner(x) is deprecated, use repository(x) instead.", :owner)
repository(x)
end
end)I only mention the latter case because I'm not sure whether the fact that the symbol :owner isn't defined in the module in which depwarn is being called matters.
a73155a to
7d0cf34
Compare
|
@simonbyrne @tkelman @ararslan still good to merge once CI passes? |
|
At sysimg compilation:
|
|
we can delete this one now that Line 1188 in b0a3ff5
|
No description provided.