Skip to content

Don't use str.find #154

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.

Sign up for GitHub

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

Jump to bottom
Merged
berkerpeksag merged 2 commits into from
Dec 8, 2019
Merged

Don't use str.find #154

berkerpeksag merged 2 commits into from
Dec 8, 2019

Conversation

@domoritz
Copy link
Contributor

@domoritz domoritz commented Aug 24, 2019

It doesn't work for unicodes in Python 2.

berkerpeksag
berkerpeksag reviewed Aug 24, 2019
View reviewed changes
Copy link
Owner

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Could you share a reproducer? It would be nice to add a test case.

@domoritz
Copy link
Contributor Author

domoritz commented Aug 24, 2019

I ran into this issue when I created an ast node in Python 2 but set the name to a unicode.

import astor
import ast
astor.to_source(ast.Name(id=u'copy', ctx=ast.Load()))

@berkerpeksag
Copy link
Owner

berkerpeksag commented Aug 25, 2019

Thanks! PR looks good to me. I'll convert your reproducer to a test case (feel free to do that if you have some spare time) and merge it.

Test failure seems unrelated as I can see it fails in other PRs:

======================================================================
FAIL: test_positional_only_arguments (tests.test_code_gen.CodegenTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/berkerpeksag/astor/tests/test_code_gen.py", line 180, in test_positional_only_arguments
    self.assertSrcRoundtrips(source)
  File "/home/travis/build/berkerpeksag/astor/tests/test_code_gen.py", line 72, in assertSrcRoundtrips
    self.assertEqual(self.to_source(ast.parse(srctxt)).rstrip(), srctxt)
AssertionError: 'def [42 chars]s\n\n\ndef test(a=3, b=4, /, c=7):\n    pass\n[40 chars]pass' != 'def [42 chars]s\n\ndef test(a=3, b=4, /, c=7):\n    pass\n\n[36 chars]pass'
  def test(a, b, /, c, *, d, **kwargs):
      pass
- 
  
  def test(a=3, b=4, /, c=7):
      pass
  
- 
  def test(a, b=4, /, c=8, d=9):
      pass

@domoritz domoritz mentioned this pull request Aug 28, 2019
Merged
@berkerpeksag berkerpeksag added this to the 0.8.1 milestone Dec 8, 2019
domoritz and others added 2 commits December 8, 2019 15:02
@domoritz @berkerpeksag
Don't use str.find
b2a8695 
It doesn't work for unicodes in Python 2.
@berkerpeksag
add test and changelog
fae3807
berkerpeksag
berkerpeksag approved these changes Dec 8, 2019
View reviewed changes
Copy link
Owner

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

LGTM! I've just added a test case and updated changelog.

@berkerpeksag berkerpeksag merged commit 0b3cbce into berkerpeksag:master Dec 8, 2019
@berkerpeksag
Copy link
Owner

berkerpeksag commented Dec 8, 2019

Thank you and sorry for my late response!

isidentical pushed a commit to isidentical/astor that referenced this pull request Dec 9, 2019
@domoritz @isidentical
Fix TypeError when passing unicode string to to_source() (berkerpeksa…
390a8f8 
…g#154)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.8.1

Development

Successfully merging this pull request may close these issues.

2 participants

@domoritz @berkerpeksag