Skip to content

fix: MockAdapter.open_modal accepts positional args#28

Merged
patrick-chinchill merged 1 commit into
mainfrom
fix/mock-open-modal
Apr 7, 2026
Merged

fix: MockAdapter.open_modal accepts positional args#28
patrick-chinchill merged 1 commit into
mainfrom
fix/mock-open-modal

Conversation

@patrick-chinchill
Copy link
Copy Markdown
Collaborator

CI was failing because open_modal(**kwargs) doesn't accept 3 positional args.

@patrick-chinchill patrick-chinchill merged commit 0e13dcf into main Apr 7, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the open_modal method in MockAdapter to use explicit parameters instead of generic keyword arguments. The review feedback recommends removing the default values for trigger_id and modal to ensure the mock correctly enforces the interface contract and prevents tests from passing when required arguments are missing.

Comment on lines +170 to +175
async def open_modal(
self,
trigger_id: str = "",
modal: Any = None,
context_id: str | None = None,
) -> dict[str, str]:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The trigger_id and modal parameters should be required (without default values) to match the interface defined in BaseAdapter and implemented in SlackAdapter. Using default values in the mock can lead to tests passing even when required arguments are missing in the production code, which defeats the purpose of the mock in catching interface contract violations.

Suggested change
async def open_modal(
self,
trigger_id: str = "",
modal: Any = None,
context_id: str | None = None,
) -> dict[str, str]:
async def open_modal(
self,
trigger_id: str,
modal: Any,
context_id: str | None = None,
) -> dict[str, str]:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant