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

KafkaConsumer.subscribe(topics='t', pattern='^t') should throw ValueError not AssertionError #891

Open
jeffwidman opened this issue Nov 29, 2016 · 2 comments

Comments

@jeffwidman
Copy link
Collaborator

jeffwidman commented Nov 29, 2016

For KafkaConsumer.subscribe() either topics or pattern must be specified but not both. While experimenting, I noticed that when I break this contract, I get an AssertionError.

Since this is caused by bad input, and not bad code, this should throw ValueError not AssertionError.

Assertions will get stripped out if python is run with the optimized flag, removing the safety net of this check. While rare in dev, some places do run python this way in production.

Happy to put together a PR if you want, but wanted to doublecheck you're on board with this change.

@dpkp
Copy link
Owner

dpkp commented Nov 29, 2016

makes sense and on board! I am a fan of asserts to document assumptions, but I think you're right that we're probably abusing them in cases like this.

@dpkp
Copy link
Owner

dpkp commented Dec 28, 2016

Also note that technically what is raised is IllegalStateError which isn't strictly an assertion. This was done to mirror the java client, which I believe raises a similar error in this case.

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

No branches or pull requests

2 participants