Resolve transcode method ambiguity
#141
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The examples in many of the codec packages rely on being able to call `transcode` providing a `Codec` type and a string[^1][^2]. JuliaIO#136 removed type annotations from the trailing arguments for `transcode(::Type{C}, ...) where {C<:Codec}` causing a method ambiguity with `transcode(T, src::String)` in Julia `Base`[^3]. This adds an additional method to `Base.transcode` to resolve this ambiguity. Fixes JuliaIO#139. [^1]: https://github.com/JuliaIO/CodecZlib.jl/tree/f9fddaa28c093c590a7a93358709df2945306bc7#usage [^2]: https://github.com/JuliaIO/CodecZstd.jl/tree/6327ffa9a3a12fc46d465dcfc8b30bed91cf284b#usage [^3]: https://github.com/JuliaLang/julia/blob/ff7b8eb00bf887f20bf57fb7e53be0070a242c07/base/c.jl#L306
bauglir
force-pushed
the
disambiguate-transcode-for-codec-types-and-strings
branch
from
52784b6 to
642db71
Compare
|
In hindsight, using |
This was referenced Apr 18, 2023
|
Any chance this can be merged and released @mkitti? |
|
I would like to go further with this. We should run |
|
|
Add this somewhere in the tests please. |
Yeah sure! I should be able to add that later today. I had held off on that because that also seems to be the point of #142. |
|
Let's just merge that then. |
|
Thanks for getting this merged. Any chance we can get a new release so downstream users do not have to work around this on v0.9.12? |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The examples in many of the codec packages rely on being able to call
transcodeproviding aCodectype and a string12. #136 removed type annotations from the trailing arguments fortranscode(::Type{C}, ...) where {C<:Codec}causing a method ambiguity withtranscode(T, src::String)in JuliaBase3. This adds an additional method toBase.transcodeto resolve this ambiguity.I have added tests for the
Noopcodec to ensure the ambiguity is resolved. I'm wondering whether it makes sense to add a test helper function that downstream packages can use to ensure this functionality remains intact. Let me know if that is something I should add.Fixes #139.
Footnotes
https://github.com/JuliaIO/CodecZlib.jl/tree/f9fddaa28c093c590a7a93358709df2945306bc7#usage ↩
https://github.com/JuliaIO/CodecZstd.jl/tree/6327ffa9a3a12fc46d465dcfc8b30bed91cf284b#usage ↩
https://github.com/JuliaLang/julia/blob/ff7b8eb00bf887f20bf57fb7e53be0070a242c07/base/c.jl#L306 ↩