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

#9 - Add support to delete publishers and subscribers from the graph #41

Merged
merged 7 commits into from
Jul 27, 2023

Conversation

ltuch
Copy link
Contributor

@ltuch ltuch commented Jul 24, 2023

No description provided.

Copy link
Member

@petehayes102 petehayes102 left a comment

Choose a reason for hiding this comment

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

Great work mate! For a complete newbie you've written some pretty idiomatic looking code.

I'm going to have a go tomorrow at refactoring your _rm_left_leaf into a single loop that recurses up the tree, deleting as it goes. We should also consider that the graph is 'live', meaning that data is flowing 'through' it while we're mutating it. I didn't put this as an acceptance criteria (sorry my bad) but we should try and isolate the branch to be deleted before operating on it.

I'll have a go at this tomorrow and then do a code review with you when I'm finished. Feel free to make the other refactoring changes in the interim.

Again, great work mate! Rust is a really tough language and you've already produced a great changeset, with tests! 🥳

server/src/graph.rs Outdated Show resolved Hide resolved
server/src/graph.rs Outdated Show resolved Hide resolved
server/src/graph.rs Outdated Show resolved Hide resolved
server/src/graph.rs Outdated Show resolved Hide resolved
server/src/graph.rs Show resolved Hide resolved
server/src/graph.rs Outdated Show resolved Hide resolved
server/src/graph.rs Outdated Show resolved Hide resolved
server/src/graph.rs Outdated Show resolved Hide resolved
server/src/graph.rs Outdated Show resolved Hide resolved
petehayes102
petehayes102 previously approved these changes Jul 27, 2023
Copy link
Member

@petehayes102 petehayes102 left a comment

Choose a reason for hiding this comment

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

I'll fix the panics on a subsequent PR

@petehayes102 petehayes102 merged commit 77c5344 into main Jul 27, 2023
3 of 4 checks passed
@petehayes102 petehayes102 deleted the delete-publishers-subscribers branch July 27, 2023 11:00
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