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.

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

Add collapse_if_tuple test cases #143

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

davesque
Copy link
Contributor

What was wrong?

The test cases for collapse_if_tuple did not cover two edge cases.

How was it fixed?

Added tests for them.

Cute Animal Picture

Cute animal picture

@davesque davesque requested a review from carver December 19, 2018 00:24
Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Nice, yeah it seemed apparent that it would work, but it's nice to have in place to prevent regressions.

@davesque davesque merged commit 5912331 into ethereum:master Dec 19, 2018
@davesque davesque deleted the collapse-if-tuple-tests branch December 19, 2018 00:36
@pipermerriam
Copy link
Member

It looks like the docs don't know anything about this function, should it be added or is it private API?

@davesque
Copy link
Contributor Author

davesque commented Dec 19, 2018

@pipermerriam Not sure. It's in a kind of weird gray area. To me, it seems rather specific to the particular problem we're trying to solve in web3.py. In that way, I feel like it doesn't belong in eth-utils. On the other hand, it is used in eth_utils.abi._abi_to_signature. It's also used in web3._utils.abi. Perhaps we could go ahead and make it private with an underscore prefix since the only place it's used outside of eth-utils is in a private utility module in web3.py.

@pipermerriam
Copy link
Member

@carver what do you think about leaving it in a weird limbo for now, not documenting publicly but exposing without an _ prefix as if it was a public API.

@davesque
Copy link
Contributor Author

@pipermerriam @carver Actually, now I don't like the idea I was proposing. It would be nice to expect that anything underscore prefixed is not used outside of a package or module.

@davesque
Copy link
Contributor Author

@pipermerriam @carver I created a separate issue ticket for this discussion.

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.

3 participants