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 new select syntax #23

Merged
merged 6 commits into from
Jul 11, 2014
Merged

Conversation

MichaelAz
Copy link
Contributor

Select can now accept *args and will throw for a combination of a list and *args as discussed in #22

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling 98e1fc9 on MichaelAz:22/new_select into e12293c on rgalanakis:master.

@MichaelAz
Copy link
Contributor Author

I'm not sure I know what you mean by "user error", but I believe that this should be an assertion for several reasons:

  1. If we were to drop the assertion, and the user was to mix a list and other arguments, no error would occur. We would allow the user to write semantically incorrect code and they wouldn't even know it.
  2. It is an established pattern in the library to explicitly assert assumptions about parameters (and assumptions in general). For example, BufferedChan checks that the size argument for __init__ is greater than or equal to zero.

Are there any other built-in exceptions that are better suited, in your opinion?
Do you think we should define our own ArgumentError?

@ctismer
Copy link
Contributor

ctismer commented Jun 27, 2014

I don't want to be boring, but it is a fact that assertions are for testing only.
For the full argumentation, you can refer to

https://mail.python.org/pipermail/python-list/2013-November/660401.html

As a short reason: your code will be unprotected, if somebody runs it with .pyo :-D
There should be no code protection that can be switched off.

ad 1.: Sure, things should and must be checked, but not with assert, but raising something explicitly.

ad 2.: As long as the assertions are checking the correctness of the implementation, they are perfectly valid, because that is meant. As soon as the assertion checks user-provided input, it is wrongly designed code.

I have no preference to introduce a new error or use a simple pre-defined one. Important is that there is always an error raised. You can even explicitly raise an AssertionError ;-)

@rgalanakis
Copy link
Owner

Sorry for the delay- I will check this out as soon as I have internet again.

@@ -54,6 +54,11 @@ def select(cases):
:return: ``(chosen case, received value)``.
If the chosen case is not an :class:`goless.rcase`, it will be None.
"""
# Sanity check - if the first argument is a list, it should be the only argument
if isinstance(cases[0], list):
Copy link
Owner

Choose a reason for hiding this comment

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

Calling select with no cases should not result in an IndexError. IMO it should just work, like it has.

@rgalanakis
Copy link
Owner

This is not really a case for an assert, this is a case for a TypeError or ValueError. Sometimes I will use assert to sanitize values, but not for public APIs.
Anyway, once the feedback is addressed I will merge this.

@rgalanakis
Copy link
Owner

@MichaelAz I'd like to get this in so I can cut a 0.6.1 release. Can you clean this up, or should I redo your changes?

@MichaelAz
Copy link
Contributor Author

I was under the impression I addressed all of your comments but I've missed that last one. I just pushed an update. Anything else or can this be merged?

@rgalanakis
Copy link
Owner

Sorry, somehow missed your recent changes.
Anyway, added one final comment. Also, the build is failing :(

@MichaelAz
Copy link
Contributor Author

Both adressed.
The reason the build failed is because I changed the assert from self.assertIs(type(chosen), goless.scase) to self.assertIs(chosen, chan1). You must have meant self.assertIs(chosen, scase) when you suggested that, and I didn't catch that error.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) when pulling 5d6ea41 on MichaelAz:22/new_select into e12293c on rgalanakis:master.

rgalanakis added a commit that referenced this pull request Jul 11, 2014
@rgalanakis rgalanakis merged commit 0f4db79 into rgalanakis:master Jul 11, 2014
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.

None yet

4 participants