-
Notifications
You must be signed in to change notification settings - Fork 250
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
Rename logger
to logger_
in slack_bolt.kwargs_injection.args.Args
#1032
Comments
Hi @ChrisHills463 - Thank you for writing in with this suggestion! Your point about improving usability seems reasonable to me, but I'll defer to @WilliamBergamin or @seratch on tradeoffs regarding making a change like this. |
We have resolved the same issue with next function this way: #394 Adding an alias in the similar way should be a reasonable solution for this need. No breaking change at all. |
Thanks for the feedback! I think that there is a slight difference in that unlike |
Yeah, there is a difference, but we never bring breaking changes to this SDK (considering the number of existing apps for enterprise companies). Thus, the only option we can think of is to add an alias to support your use case. |
Understood, but adding an alias would be pointless because the original |
Just to clarify, bolt-python's keyword argument injection mechanism adds arguments only when you give the name in the argument list (see https://github.com/slackapi/bolt-python/blob/main/slack_bolt/kwargs_injection/utils.py to learn how it works). Therefore, when you have |
Thank you for the clarification! This has cleared up my misunderstanding. |
It is common practice to define a variable called
logger
in a script. However, theslack_bolt.kwargs_injection.args.Args
function used as a decorator also defineslogger
, so if you want to call your normal logger in your script you have to give it a different name. It can also be unexpected behaviour, as the author may think they are calling thelogger
from their module, but actually they are calling a different logger instance altogether.I understand this would be a breaking change, but I think it would improve usability.
Category (place an
x
in each of the[ ]
)Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
The text was updated successfully, but these errors were encountered: