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

Deprecate LibGit2.owner in favour of repository #20135

Merged
merged 1 commit into from
Jan 23, 2017
Merged

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Jan 20, 2017

No description provided.

@kshyatt kshyatt added deprecation This change introduces or involves a deprecation libgit2 The libgit2 library or the LibGit2 stdlib module labels Jan 20, 2017
@@ -1758,4 +1758,7 @@ end)
@deprecate(SharedArray{T}(filename::AbstractString, ::Type{T}, dims::NTuple, offset; kwargs...),
SharedArray{T,length(dims)}(filename, dims, offset; kwargs...))

# LibGit2.owner -> LibGit2.repository for consistency
eval(Base.LibGit2, :(Base.@deprecate_binding owner repository))
Copy link
Member

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)))

Copy link
Member

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.

Copy link
Contributor

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?

@tkelman
Copy link
Contributor

tkelman commented Jan 20, 2017

was owner previously exported from the libgit2 module? if not, this would cause it to be exported which you probably don't want

@kshyatt
Copy link
Contributor Author

kshyatt commented Jan 20, 2017

It wasn't, I don't think. How can I deprecate it without exporting it?

@tkelman
Copy link
Contributor

tkelman commented Jan 20, 2017

manually call depwarn. simon has a commit in one of his pr's that adds a kwarg optional argument to disable exporting for at-deprecate, but deprecate_binding is separate

@@ -1763,4 +1763,13 @@ end)
return false
end

# LibGit2.owner -> LibGit2.repository for consistency
eval(Base.LibGit2, :(Base.@deprecate_binding owner repository))
Copy link
Contributor

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

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
Copy link
Member

@ararslan ararslan Jan 23, 2017

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)
end

The 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.

@kshyatt
Copy link
Contributor Author

kshyatt commented Jan 23, 2017

@simonbyrne @tkelman @ararslan still good to merge once CI passes?

@kshyatt kshyatt merged commit 08adcd3 into master Jan 23, 2017
@kshyatt kshyatt deleted the ksh/ownerrepo branch January 23, 2017 21:46
@KristofferC
Copy link
Member

KristofferC commented Feb 1, 2017

At sysimg compilation:

WARNING: Method definition owner(Any) in module LibGit2 at deprecated.jl:1807 overwritten at deprecated.jl:49.

@tkelman
Copy link
Contributor

tkelman commented Feb 2, 2017

we can delete this one now that

@deprecate owner(x) repository(x) false
does it more concisely

@Sacha0 Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants