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

Enable developers to inject non-bolt arguments to listener function args #709

Open
cooperbenson-qz opened this issue Aug 24, 2022 · 7 comments
Assignees
Milestone

Comments

@cooperbenson-qz
Copy link

I'm using dependency_injector to provide automatic dependency injection for my application, and I'm encountering an issue with Bolt overriding injected parameters with None since it doesn't recognize them.

Reproducible in:

pip freeze | grep slack
python --version
sw_vers && uname -v # or `ver`

The slack_bolt version

slack-bolt==1.14.3
slack-sdk==3.18.1

Python runtime version

Python 3.10.6

OS info

ProductName:    macOS
ProductVersion: 12.5.1
BuildVersion:   21G83
Darwin Kernel Version 21.6.0: Wed Aug 10 14:28:23 PDT 2022; root:xnu-8020.141.5~2/RELEASE_ARM64_T6000

Steps to reproduce:

(Share the commands to run, source code, and project settings (e.g., setup.py))

  1. Have a kwarg that is populated by some mechanism other than bolt (such as another decorator), e.g.
@app.command('/ping')
@inject
async def handle_ping(ack, respond, version: str = Provide[Container.config.VERSION]):
    await ack()
    await respond({
        "blocks": [
            SectionBlock(text=MarkdownTextObject(text='Pong!')),
            DividerBlock(),
            ContextBlock(elements=[
                MarkdownTextObject(text=f'*Host:* {platform.node()}'),
                MarkdownTextObject(text=f'*Version:* {version}')
            ])
        ]
    })

Expected result:

Bolt doesn't touch the version kwarg, but still injects the ack and respond kwargs.

Actual result:

version ends up being set to None and Bolt logs the following warning: version is not a valid argument.

This is caused by this line.

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.

@cooperbenson-qz cooperbenson-qz changed the title How to allow unknown kwargs How do I allow unknown kwargs in listener funcs? Aug 24, 2022
@filmaj
Copy link
Contributor

filmaj commented Aug 24, 2022

Seems like a reasonable request to remove that part of the code. @seratch is the kwargs scrubbing code that @cooperbenson-qz linked to necessary to keep?

@cooperbenson-qz
Copy link
Author

I'm happy to make a PR based on whatever determination you come to 😁

@seratch
Copy link
Member

seratch commented Aug 24, 2022

I agree that we can stop explicitly setting None to unknown args there, but I still would like to maintain the default behavior to print the warning logs. The log intends to make the dev experience more friendly by informing misspellings/typos of argument names.

Perhaps, adding a new option to disable the check to App initialization may be a good solution for this use case.

@seratch seratch modified the milestones: 1.14.4, 1.15.0 Aug 24, 2022
@eddyg
Copy link
Contributor

eddyg commented Aug 24, 2022

I like the idea of needing to specify an option to disable the "not a valid argument" warning message in situations where the framework is being used in an "expert" manner.

(I don't know if there are other similar log messages in the code where it might be nice to suppress them in less-typical use cases. If there are, naming the option something "generic" might be worthwhile so the option can be used in multiple places, instead of adding one for each warning whenever a situation like this comes up.)

@cooperbenson-qz
Copy link
Author

cooperbenson-qz commented Aug 24, 2022

Perhaps instead of using the logger to output the warning message, it might make sense to use warnings since that gives the user a lot of options for filtering and surpassing warnings.

@seratch seratch self-assigned this Aug 25, 2022
@seratch seratch changed the title How do I allow unknown kwargs in listener funcs? Enable developers to inject non-bolt arguments to listener function args Aug 25, 2022
seratch added a commit to seratch/bolt-python that referenced this issue Aug 25, 2022
@seratch
Copy link
Member

seratch commented Aug 25, 2022

@eddyg @cooperbenson-qz Thanks a lot for sharing your great insights. Indeed, using warnings for this purpose makes sense. Please feel free to add comments to #712 if you have any.

@seratch seratch modified the milestones: 1.15.0, 1.x Aug 25, 2022
@seratch
Copy link
Member

seratch commented Aug 25, 2022

Sorry. After working on #712 a bit more, I've concluded my solution can't work at all and I cannot think of any elegant solutions for it. I myself am not going to use my time for it anymore.

If someone can enhance bolt-python to support the use cases mentioned here without breaking any existing code, we are happy to review the changes.

Until then, a workaround that I suggest is:

@inject
def initialize_listeners(
    app: AsyncApp,
    # Inject your components here
    version: str = Provide[Container.config.VERSION],
):
    @app.command('/ping')
    async def handle_ping(ack, respond):
        await ack()
        await respond({
            "blocks": [
                SectionBlock(text=MarkdownTextObject(text='Pong!')),
                DividerBlock(),
                ContextBlock(elements=[
                    MarkdownTextObject(text=f'*Host:* {platform.node()}'),
                    MarkdownTextObject(text=f'*Version:* {version}')
                ])
            ]
        })

app = AsyncApp()
initialize_listeners(app)
if __name__ == "__main__":
    app.start()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants