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

refactor: change Node pointers to values & return (Node, bool) #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

topi314
Copy link
Contributor

@topi314 topi314 commented Nov 10, 2024

This changes the method receivers of the Node type to values instead of pointers for non mutating methods & pointer return type as discussed in #19 (comment)

@topi314 topi314 marked this pull request as draft November 10, 2024 02:41
@topi314 topi314 force-pushed the refactor/node-value-receivers branch from b4b762c to 0e66c09 Compare November 10, 2024 11:35
query.go Show resolved Hide resolved
query.go Show resolved Hide resolved
tree.go Show resolved Hide resolved
tree.go Show resolved Hide resolved
@topi314 topi314 marked this pull request as ready for review November 10, 2024 11:39
@topi314 topi314 force-pushed the refactor/node-value-receivers branch 2 times, most recently from 839edd6 to f22ab79 Compare November 24, 2024 23:25
@topi314
Copy link
Contributor Author

topi314 commented Dec 14, 2024

any chance you have some time to look at this? @amaanq

@amaanq
Copy link
Member

amaanq commented Jan 29, 2025

Sorry @topi314, I think after thinking about this for a bit, it makes total sense to have the node methods pass the object by value, since none of them besides Edit mutate the Node, and they're pretty small, stack-allocated objects - with that, I would think it'd be more natural if newNode returned a tuple of (Node, bool), indicating if the Node is empty or not. Would you be up for adding that change in this PR too?

@topi314
Copy link
Contributor Author

topi314 commented Jan 29, 2025

hey @amaanq, thanks for reconsidering on this matter. Yes I'd love to implement this and will try to do it in the next days.

@topi314 topi314 force-pushed the refactor/node-value-receivers branch from f22ab79 to d7ee56b Compare February 2, 2025 22:39
@topi314 topi314 changed the title refactor: change Node pointers to values refactor: change Node pointers to values & return (Node, bool) Feb 2, 2025
@topi314 topi314 force-pushed the refactor/node-value-receivers branch from d7ee56b to 24f70e3 Compare February 2, 2025 22:41
@topi314
Copy link
Contributor Author

topi314 commented Feb 2, 2025

I think this should be all changes required for refactoring.

I don't particularly like how I've written the tests, but this is prob the best we can do.
If you have any better idea let me know

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.

2 participants