bolt-python icon indicating copy to clipboard operation
bolt-python copied to clipboard

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

Open cooperbenson-qz opened this issue 3 years ago • 7 comments

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 avatar Aug 24 '22 16:08 cooperbenson-qz

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?

filmaj avatar Aug 24 '22 18:08 filmaj

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

cooperbenson-qz avatar Aug 24 '22 20:08 cooperbenson-qz

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 avatar Aug 24 '22 21:08 seratch

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.)

eddyg avatar Aug 24 '22 21:08 eddyg

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.

cooperbenson-qz avatar Aug 24 '22 21:08 cooperbenson-qz

@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 avatar Aug 25 '22 06:08 seratch

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()

seratch avatar Aug 25 '22 09:08 seratch