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

Rename logger to logger_ in slack_bolt.kwargs_injection.args.Args #1032

Closed
1 of 4 tasks
ChrisHills463 opened this issue Feb 22, 2024 · 7 comments
Closed
1 of 4 tasks
Labels
enhancement New feature or request good first issue

Comments

@ChrisHills463
Copy link

It is common practice to define a variable called logger in a script. However, the slack_bolt.kwargs_injection.args.Args function used as a decorator also defines logger, 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 the logger 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 [ ])

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Others

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.

@srajiang srajiang added enhancement New feature or request auto-triage-skip labels Feb 22, 2024
@srajiang
Copy link
Contributor

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.

@seratch
Copy link
Member

seratch commented Feb 22, 2024

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.

@seratch seratch added this to the 1.18.2 milestone Feb 22, 2024
@ChrisHills463
Copy link
Author

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 next, logger is not a Python built-in but rather a common convention, which is why I think it should be renamed to logger_ since when an author refers to logger they are likely thinking that they are referring to their local module variable rather than that of the (decorated) function.

@seratch
Copy link
Member

seratch commented Feb 23, 2024

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.

@ChrisHills463
Copy link
Author

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 logger is already overwritten at this point, so I think at this point there is no alternative to the author using a name other than logger, or providing their own alias to it.

@seratch
Copy link
Member

seratch commented Feb 23, 2024

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 logger outside the listener function and you give only logger_ in the listener method's argument list, you can use both the global logger and bolt's logger as logger_ within the listener function. If this does not work for your use case, we may not add such change because we've never received this feedback from other developers.

@ChrisHills463
Copy link
Author

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 logger outside the listener function and you give only logger_ in the listener method's argument list, you can use both the global logger and bolt's logger as logger_ within the listener function. If this does not work for your use case, we may not add such change because we've never received this feedback from other developers.

Thank you for the clarification! This has cleared up my misunderstanding.

@seratch seratch removed this from the 1.18.2 milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue
Projects
None yet
Development

No branches or pull requests

3 participants