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

Don't rely on implicit binding creation of setglobal! #121

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

Keno
Copy link
Contributor

@Keno Keno commented Jun 6, 2024

The mod.sym = val syntax is not supposed to be able to create new bindings if mod.sym does not yet exist. The error check for this was accidentally dropped in Julia 1.9, but will likely be put back in 1.11 [1]. This uses Core.eval/invokelatest to achieve the same effect using the recommeded replacement. As a general note, the way this package uses bindings is clever, but not particularly idiomatic in Julia. Not saying you can't do it, but I would recommend revisiting if at least the injected bindings could perhaps be replaced by something like an IdDict.

[1] JuliaLang/julia#54678

The `mod.sym = val` syntax is not supposed to be able to create new bindings
if `mod.sym` does not yet exist. The error check for this was accidentally
dropped in Julia 1.9, but will likely be put back in 1.11 [1]. This uses
Core.eval/invokelatest to achieve the same effect using the recommeded
replacement. As a general note, the way this package uses bindings is
clever, but not particularly idiomatic in Julia. Not saying you can't
do it, but I would recommend revisiting if at least the injected bindings
could perhaps be replaced by something like an IdDict.

[1] JuliaLang/julia#54678
Copy link
Member

@emmaccode emmaccode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
I will probably just be wrapping server, procman and data into a new ServerController something or other return type, rather than putting them into the Module.

Thanks for the info, good to know about that before I upgrade and figure out for myself.

Makes me wonder how the GenieRouter loads routes. Maybe I will look into that.

@emmaccode emmaccode merged commit aeeaebd into ChifiSource:main Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants