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.

Sign up for GitHub

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

Jump to bottom

feat(handler): add index field to support chained handlers #343

Merged
merged 1 commit into from
May 26, 2021
Merged

feat(handler): add index field to support chained handlers #343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions queenbee/io/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,3 +410,12 @@ class IOAliasHandler(BaseModel):
description='Name of the function. The input value will be passed to this '
'function as the first argument.'
)

index: int = Field(
0,
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we can replace this a nullable field rather than a default one? This will make the recipe digest more reliable when comparing with digests already present on the database.

index: Optional[int] = Field(None, description='An integer...'

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @AntoineDao, Thanks for the note. I should have waited for your before merging this in. I remember we had issues with Optional when we used it in Honeybee Schema: ladybug-tools/honeybee-schema#121 (comment) - optional doesn't really translate to OpenAPI and that can cause inconsistencies.

I know that this might generate a new hash for the current packages the first time but it will be fine after that.

Copy link
Member

Choose a reason for hiding this comment

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

I think waiting would have been helpful here. I'm a bit nervous about what will happen when we merge this into pollination-server so I will push it now and we can keep an eye on it to see if it breaks anything

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! Based on what we talked about before - adding new optional fields should be fine but you never know. 🤞

description='An integer to set the index for the order of execution. This input '
'is only useful when there are more than one handler for the same platform and '
'the output of one handler should be passed to another handler. This is also '
'called chained handlers. By default all the handlers are indexed as 0 assuming '
'they are not chained.'
)