Skip to content

Conversation

davidmrdavid
Copy link
Collaborator

@davidmrdavid davidmrdavid commented Jul 15, 2020

Addresses: #155

This PR represented a simple change with important consequences, and some implications about coding style. Before, orchestrators failed to serialize their return value when it's value was False. The reason why is that we performed a faulty check in our serialization logic. The check was to avoid serializing a None value, but it was written as:

if self._output:
    # serialize

In most cases, this is fine, because None values resolve to False. However, this broke when self._output was literally False. A safer check is if self._output is not None. We should consider a refactor where we rewrite all these "dangerous" checks into their safer alternative.

I'm tracking that task here: #166

@davidmrdavid davidmrdavid changed the title WIP: Serialize False as a return value for orchestrators Serialize False as a return value for orchestrators Jul 17, 2020
Copy link

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

The code looks right, but an example test that returns False that would break without this change would be preferable before merging.

@davidmrdavid davidmrdavid merged commit 351dd93 into dev Jul 24, 2020
@davidmrdavid davidmrdavid deleted the dajusto/serialize-falses branch July 24, 2020 17:30
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