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

Node comparison: == on nodes now checks contents #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ze42
Copy link

@ze42 ze42 commented Jun 17, 2014

Same as comparing fst, but without building each explicitly first

Same as comparing fst, but without building each explicitly first
@Psycojoker
Copy link
Member

Hum ... this doesn't compare the parent/on_attribute attributes, I need to think if this is the kind of behavior that will be expected by the users. Don't hesitate to voice your opinion.

I wonder about when someone would do this kind of comparison between nodes.

Thanks for your work :)

@ze42
Copy link
Author

ze42 commented Jun 18, 2014

red[0] == red[0].copy()

If you expect True, you can't be checking parents, as the copy does NOT have any parent.

Not checking the parent might allow to compare class/functions from different files, without checking the surroundings.

Do you have any case where you might want to be also comparing where the node is? If you do, you'll probably end up with something that can't be so easily recursive anyway, without risking a recursive loop.

@ibizaman
Copy link
Collaborator

I also vote for not checking the parent. I'm less sure for the on_attribute though.
I agree with @ze42's arguments. There's one more: to conform with baron's node comparison behaviour.

In baron, comparing two nodes cannot compare the parents since you cannot go up the tree. I would expect something similar in redbaron.

And if you still want to compare the parents, you can:

red1 = RedBaron("a = 1")
node1 = red1[0].target

red2 = RedBaron("a = 1")
node2 = red2[0].target

assert node1 is not node2
assert node1 == node2 and node1.path() == node2.path()

@ze42
Copy link
Author

ze42 commented Jun 18, 2014

Yeah, Path comparison is a nice idea.

node1.path() == node1.path() was returning False

Added a fix so path can also be compared, your example working properly on it.

@ibizaman
Copy link
Collaborator

@ze42 nice, I didn't even think to check if my example was working /o\

@ze42
Copy link
Author

ze42 commented Jun 18, 2014

Yeah, it's a good think to first write what we want, before checking if the code is ok to make it work :)

@Psycojoker
Copy link
Member

Those arguments makes sens, let's go for that :)

This pull request still needs some tests (in tests/test_redbaron.py) and some documentation before merging. I'm coming back home tomorrow, I'll review the code then (also rtfm python comparisons behavior).

@Psycojoker
Copy link
Member

Code is nice, thanks for that :)

I've pushed some tests here, they should end up in this pull request (or you can write some others if you want to) https://github.com/Psycojoker/redbaron/blob/085eb5ea30e8faaee65ace86ae92d9538f604c33/tests/test_redbaron.py#L1139-1170

I've also added a CHANGELOG, can you add one bullet point in it for the next release stating that comparison between 2 nodes is implemented plz? https://github.com/Psycojoker/redbaron/blob/master/CHANGELOG you can use this other changelog for inspiration https://github.com/Psycojoker/baron/blob/master/CHANGELOG Thanks in advance!

Btw, we still need to write this new behavior somewhere in the doc, probably in the "other.rst" file.

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