Skip to content

[Proposal / RFC] Align Mixin instance_init signature with Invocable instance_init signature#522

Merged
eob merged 6 commits into
mainfrom
fix-configs
Aug 18, 2023
Merged

[Proposal / RFC] Align Mixin instance_init signature with Invocable instance_init signature#522
eob merged 6 commits into
mainfrom
fix-configs

Conversation

@eob
Copy link
Copy Markdown
Contributor

@eob eob commented Aug 18, 2023

Context

We've been discussing ways to simplify the Config handling requirements across mixins and packages.

One complexity identified is that configs of different types were being passed around automatically at different times.

This PR

This PR removes all arguments from the Mixin's instance_init method, bringing it into alignment with PackageService.

This simplifies the contract of a Mixin's state without impacting possibility: the state would not be provided exclusively upon instantiation, by the PackageService author. 100% of the state it needs must be provided at that time.

Mixins affected

This only affects the Telegram mixin, but it turns out that that Mixin already had -- via its __init__ method -- all of the information that it just lost by simplifying the instance_init method.

Implied "Best Practice" Mixin Configuration Pattern

This results in a situation where we do not need to commingle MixinConfig and PackageConfig.

The following manner of configuration can now keep the two configuration layers completely separate, ducking around multi-inheritance issues with Pydantic:

            TelegramTransport(
                client=self.client,
                config=TelegramTransportConfig(bot_token=self.config.bot_token),
            )

@eob eob requested a review from dkolas August 18, 2023 15:20
dkolas
dkolas previously approved these changes Aug 18, 2023
Copy link
Copy Markdown
Contributor

@dkolas dkolas left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me

dkolas
dkolas previously approved these changes Aug 18, 2023
Copy link
Copy Markdown
Contributor

@dkolas dkolas left a comment

Choose a reason for hiding this comment

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

🚢 :shipit:

@eob eob added this pull request to the merge queue Aug 18, 2023
Merged via the queue into main with commit dab89fd Aug 18, 2023
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.

2 participants