-
Notifications
You must be signed in to change notification settings - Fork 65
Make @service
only define a module once
#690
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
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM
Do you have a scenario in which you want to call The only reason I can think of to support multiple |
I agree that the case this facilitates, dynamically changing the features in use for a given service, seems like an anti-pattern within a package. I'd be surprised if anybody would actually do that. The non-interactive use case I could imagine would be for folks who like to include all of their dependency loading at the top of each file in a package that utilizes those dependencies. (Not particularly common but I have seen it.)
I would consider this the primary use case. |
Currently, `@service` defines a module every time it's called. That works fine but the downside is that you get module redefinition warnings if you happen to call `@service` again. The most plausible reason why one would want to redefine the module would be to enable/disable particular features in some region of code. To facilitate that use case while avoiding redefinition warnings, we can `@service` check for an existing module, define one if it can't find one, and if it can then modify the feature set associated with the module in place. With this change, `@service S3` (e.g.) effectively expands to: ```julia if isdefined(Main, :S3) AWS.set_features!(S3.SERVICE_FEATURE_SET) else module S3 # contents end end ``` The `set_features!` call uses the features specified in the macro call as keyword arguments. No arguments uses defaults. Notably this design requires that `FeatureSet` be made mutable, which seems fine IMO.
6c78462
to
3e63626
Compare
Thanks for updating the PR. I'll do some local experimentation with this yet. I expect this will be merged by or on Monday |
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.
I'm fine with this for supporting interactive usage but for package development modifying the global feature set for the @service
module is dangerous as this is effectively modifying a global which changes the behaviour of all function calls using the service module.
I'm not seeing what problem this is solving beyond hiding a warning which can now be alternatively solved with using the import as syntax: @service STS as A use_response_type=true; @service STS as B use_response_type=false
.
I see two options to move forward:
- We document the limitations of using
@service
on the same module name. - We update the examples in
@service
to show how to use the same service with different feature sets enabled.
@service S3 use_response_type = true | ||
features = S3.SERVICE_FEATURE_SET | ||
@service S3 | ||
@test features === S3.SERVICE_FEATURE_SET # ensures module wasn't replaced |
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.
I'm surprised that the containing testset captures this test.
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.
¯\_(ツ)_/¯
💯 this should only ever be used interactively. Perhaps some strong wording should be added to the documentation to that effect.
The annoyance is needing to assign a different name during interactive use. The situation is somewhat analogous to how before Julia 1.12 you couldn't redefine a type at the REPL, so you needed to use a different name if you modified the structure at all. Admittedly that case is a bit different since it was an error rather than a warning, but I see no reason not to make things in this case friendlier for interactive use. |
@@ -168,6 +179,9 @@ AWS.Response: application/xml interpreted as: | |||
OrderedCollections.LittleDict{Union{String, Symbol}, Any, Vector{Union{String, Symbol}}, Vector{Any}} with 2 entries: | |||
"GetCallerIdentityResult" => LittleDict{Union{String, Symbol}, Any, Vector{Un… | |||
"ResponseMetadata" => LittleDict{Union{String, Symbol}, Any, Vector{Un… | |||
|
|||
julia> @service STS as SecurityTokenService # reset a feature to its default by calling `@service` again |
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.
julia> @service STS as SecurityTokenService # reset a feature to its default by calling `@service` again | |
julia> @service STS as SecurityTokenService # for interactive use, call `@service` again to reset a feature to its default |
Currently,
@service
defines a module every time it's called. That works fine but the downside is that you get module redefinition warnings if you happen to call@service
again. The most plausible reason why one would want to redefine the module would be to enable/disable particular features in some region of code. To facilitate that use case while avoiding redefinition warnings, we can make@service
check for an existing module, define one if it can't find one, and if it can then modify the feature set associated with the module in place. With this change,@service S3
(e.g.) effectively expands to:The
set_features!
call uses the features specified in the macro call as keyword arguments. No arguments uses defaults. Notably this design requires thatFeatureSet
be made mutable, which seems fine IMO.