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

Issue #117: Fix bug with finding comment node #118

Merged
merged 1 commit into from Dec 22, 2016
Merged

Issue #117: Fix bug with finding comment node #118

merged 1 commit into from Dec 22, 2016

Conversation

gtors
Copy link
Contributor

@gtors gtors commented Aug 30, 2016

Added find_iter method, which is became base for find and
find_all methods.

@b5y
Copy link
Contributor

b5y commented Aug 30, 2016

You should start from baron. Take a glance for #95 issue.

Added `find_iter` method, which is became base for `find` and
`find_all` methods.
@gtors
Copy link
Contributor Author

gtors commented Aug 30, 2016

Before my changes:

r = RedBaron("""
class X:
    # test
    def __init__(self): 
        pass
""")
r.find('comment') #  None <---------- Wrong! Expected CommentNode here

After my changes:

r = RedBaron("""
class X:
    # test
    def __init__(self): 
        pass
""")
r.find('comment') # '#test' <---------- OK! As expected, CommentNode is found. 

@b5y
Copy link
Contributor

b5y commented Aug 30, 2016

You should add test cases. Without them it is difficult to say something about code. I advise you to check another nodes with find and find_all methods.

@gtors
Copy link
Contributor Author

gtors commented Aug 30, 2016

@b5y
Copy link
Contributor

b5y commented Aug 30, 2016

@gtors Okay. In first commit I did not see it.

@bootandy
Copy link

bootandy commented Dec 2, 2016

Is this getting merged in ? I just encounted a similar comment parsing bug. @b5y

Copy link
Collaborator

@ibizaman ibizaman left a comment

Choose a reason for hiding this comment

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

Everything looks good. So two things take place in the same commit.
First, refactoring find and find_all in find_iter which is super neat.
Second, fixing the comment issue. Thanks for adding tests for that! But can you link to the exact line(s) in the commit that fixes it? My eye can't find it.

Also, could you add two lines to the CHANGELOG explaining the new find_iter function and the fix as well as adding the find_iter function in the documentation?

@gtors
Copy link
Contributor Author

gtors commented Dec 20, 2016

But can you link to the exact line(s) in the commit that fixes it? My eye can't find

AFAICR, find and find_all have different behaviors:
L671
L766
The fix was in eliminating these differences.

Also, could you add two lines to the CHANGELOG explaining the new find_iter function and the fix as well as adding the find_iter function in the documentation?

Ok, but as soon as I have some free time.

@ibizaman
Copy link
Collaborator

ibizaman commented Dec 21, 2016

The fix was in eliminating these differences.

Nice catch!

Ok, but as soon as I have some free time.

@gtors no worries, I'll add the changes myself

@Psycojoker
Copy link
Member

Fix released https://pypi.python.org/pypi/redbaron/0.6.3

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

5 participants