-
Notifications
You must be signed in to change notification settings - Fork 65
Avoid extending Type
from Base on Julia 1.12
#543
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
- Coverage 87.43% 86.96% -0.48%
==========================================
Files 26 27 +1
Lines 3288 3398 +110
==========================================
+ Hits 2875 2955 +80
- Misses 413 443 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Same problem on Julia 1.12-beta3 |
Could you rebase on main to re-run CI? |
ee4da1c
to
c7b40c4
Compare
@kou, I've rebased. |
Hmm... CI isn't executed... Could you re-rebase...? |
For some reason, the internal Flatbuf module defines a function called `Type`. On Julia 1.12, defining a new function by defining methods is insufficient to distinguish it from defining new constructor methods for a visible type of the same name. It's now necessary to forward-declare the function to ensure it takes precedence over the other visible binding. Without doing so, `Base.Type` is extended in Flatbuf instead.
c7b40c4
to
9eb80c1
Compare
Done, but I still don't see CI status. Is the repository configured such that CI is not run on PRs from forks? |
Thanks. I can approve CI execution for "first-time contributor" now. I've approved now. Let's see the CI result with this change. |
Looks like CI fails on Julia nightly with multiple threads, but not in a way that could plausibly be related to the changes here, so I think we're okay. |
@quinnj @ericphanson @baumgold Can we merge this? |
@quinnj @ericphanson @baumgold I'll merge this in the next week if nobody objects it. |
@kou please merge since 1.12 is now RC2 ❤️ |
Merged. Should we release a new version before Julia 1.12 release? |
@kou yes! |
OK. Let's discuss it in #567. |
The internal Flatbuf module defines a function called
Type
. On Julia 1.12, defining a new function by defining methods is insufficient to distinguish it from defining new constructor methods for a visible type of the same name. It's now necessary to forward-declare the function to ensure it takes precedence over the other visible binding. Without doing so,Base.Type
is extended in Flatbuf instead.On current
main
with Julia v1.13.0-DEV.122:With this PR, the output is the same because of JuliaLang/julia#57546 but will have the correct behavior once that issue is fixed.
Fixes #555