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

Fix undefined name uri -> parsed_uri in smart_open_lib #214

Merged
merged 1 commit into from
Aug 7, 2018

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Aug 7, 2018

Fixes #213

NameError would be raised in these instances instead of the intended RuntimeError because uri is an undefined name in these contexts.

Fixes piskvorky#213

__NameError__s would be raised in these instances instead of the intended __RuntimeError__s because __uri__ is an _undefined name_ in these contexts.
@piskvorky
Copy link
Owner

piskvorky commented Aug 7, 2018

Thanks @cclauss .

How did this bug get in? @menshikh-iv @mpenkov Now I'm wondering if there may be other cases, perhaps introduced by the same refactoring…?

@cclauss can you add a unit test, to make sure we prevent regressions?
Something that fails BEFORE this PR, and succeeds after.

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Aug 7, 2018

@piskvorky not refactoring, this happens in #191.
Current kind of error can be simply detected by flake8 (or any other style-checker), I don't think that we need tests here, we simply need to add style-checker to smart_open, let me create an issue.

UPD: issue created #216

@menshikh-iv menshikh-iv changed the title Undefined name: uri --> parsed_uri in smart_open_lib.py Fix undefined name uri -> parsed_uri in smart_open_lib Aug 7, 2018
@menshikh-iv
Copy link
Contributor

@cclauss thanks for fix, congratz with your first contribution 👍

@menshikh-iv menshikh-iv merged commit 20c6c93 into piskvorky:master Aug 7, 2018
@cclauss cclauss deleted the patch-1 branch August 7, 2018 15:04
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