Skip to content

Bug/sc 133490 stable output schema#37

Merged
alexbourret merged 6 commits intorelease/1.2.0from
bug/sc-133490-stable-output-schema
Oct 11, 2023
Merged

Bug/sc 133490 stable output schema#37
alexbourret merged 6 commits intorelease/1.2.0from
bug/sc-133490-stable-output-schema

Conversation

@alexbourret
Copy link
Copy Markdown
Collaborator

No description provided.

@shortcut-integration
Copy link
Copy Markdown

This pull request has been linked to Shortcut Story #133490: [api-connect] get a stable output schema.

@alexbourret alexbourret changed the base branch from release/1.2.0 to master May 31, 2023 11:19
@alexbourret alexbourret changed the base branch from master to release/1.2.0 May 31, 2023 11:19
Copy link
Copy Markdown

@Alexlandeau Alexlandeau left a comment

Choose a reason for hiding this comment

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

Small nitpick on behaviour on error default value for legacy recipes, and broken for python3.7 but already discussed

logger.info("config={}".format(logger.filter_secrets(config)))

credential_parameters = config.get("credential", {})
behaviour_when_error = config.get("behaviour_when_error", "add-error-column")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🥜 nitpick: as discussed, maybe we can have a default to add-error-column for previously-existing recipes, and keep-error-column else

self.timeout = endpoint.get("timeout", -1)
if self.timeout > 0:
self.requests_kwargs.update({"timeout": self.timeout})
self.behaviour_when_error = behaviour_when_error or "add-error-column"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same feedback regarding default value

self.maximum_number_rows = maximum_number_rows
self.is_row_limit = (self.maximum_number_rows > 0)
self.can_raise = False
self.behaviour_when_error = behaviour_when_error or "add-error-column"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same feedback regarding default value

"label": "Error behaviour",
"description": "Decide how the recipe should react when an input line results in an error",
"type": "SELECT",
"defaultValue": "keep-error-column",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🥜 nitpick: why have keep-error-column as default when add-error-column is set everywhere else ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is the way to enforced the new preferred behavior on new flows while keeping the same behavior on preexisting flows.
This selector did not exist in the previous version, so is not saved in existing flows that are already using the plugin. As a result, the behavior of existing flows will not be changed, because in absence of the behaviour_when_error parameter, add-error-column(the previous behavior) is the default defined in the code. When a new recipe is created, the new behavior is the one set by default in the UI, the preferred and more stable "keep-error-column"

Copy link
Copy Markdown

@Alexlandeau Alexlandeau Oct 10, 2023

Choose a reason for hiding this comment

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

Ok, no problem then 👌

@Alexlandeau Alexlandeau self-requested a review October 10, 2023 10:10
Copy link
Copy Markdown

@Alexlandeau Alexlandeau left a comment

Choose a reason for hiding this comment

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

LGTM now !

@alexbourret alexbourret merged commit d8def4f into release/1.2.0 Oct 11, 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