Skip to content

Conversation

asottile
Copy link
Contributor

No description provided.

if six.PY2:
return res.encode("UTF-8")
else:
return res
Copy link
Member

Choose a reason for hiding this comment

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

can you make it one return?

Copy link
Member

Choose a reason for hiding this comment

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

one less line

        if six.PY2:
            res = res.encode("UTF-8")
        return res

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if else is easier for humans to parse as it's clearer that there are two disparate branches -- for maintenance reasons it's also easier to update if functionality were to be added as it's clearer it needs to happen at two places at a glance.

I've seen too many bugs from using your suggested style 😆

Copy link
Member

Choose a reason for hiding this comment

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

something must be wrong with me then because for me two returns are a lot harder to understand, reason and parse 🤔 I think it's harder to maintain as now I need to update in two locations while I can clearly reduce the second to one, needing to worry only about a single case...

Copy link
Contributor Author

@asottile asottile Apr 16, 2019

Choose a reason for hiding this comment

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

your suggested pattern also makes mypy unhappy since it changes the type of the variable

Incompatible types in assignment (expression has type "bytes", variable has type "str")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol it's always something >.<

Copy link
Member

Choose a reason for hiding this comment

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

it worked a while back now it's missing 💭

Copy link
Member

Choose a reason for hiding this comment

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

microsoft/azure-pipelines-tasks#9962 turns out the ability to upload coverage report was removed 😆

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoa that's cool

@gaborbernat
Copy link
Member

Seems a flaky test 😢

@gaborbernat gaborbernat merged commit 4c3a891 into tox-dev:master Apr 16, 2019
@asottile asottile deleted the fix_runresult branch April 16, 2019 15:22
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