Make NodeID generic to allow users to "tag" different tree structures#49
Make NodeID generic to allow users to "tag" different tree structures#49m-peeler wants to merge 4 commits intorust-scraper:masterfrom
NodeID generic to allow users to "tag" different tree structures#49Conversation
Created ability to use custom ID types to "tag" different trees, creating type-level protectation against using IDs from one tree on another when working with a set of ego trees. Added new proc macro to create custom ID types and updated unit tests to include tests of transmuting trees of one ID type to another.
Created ability to use custom ID types to "tag" different trees, creating type-level protectation against using IDs from one tree on another when working with a set of ego trees. Added new proc macro to create custom ID types and updated unit tests to include tests of transmuting trees of one ID type to another.
|
Just a little background here - I've been developing something which uses a set of trees which represent different information and found that I was regularly running into issues with ambiguity in the ID types between the trees. I forked the repo for my own use to create type-level protections against this sort of thing, and figured I'd offer back the changes I made to in the event they were useful to others. |
This is not correct, adding a new generic parameter even with a default is a breaking change. More generally speaking, I am highly sceptical of the maintenance cost this introduces to the crate versus its limited benefit for most users. If you want to tag ID from different trees, wrapping them in new types should suffice (there exist proc macro crates which help with the boiler plate). |
|
I've not encountered any breaking changes, and didn't have to do any refactors within the tests to ensure continued functionality. What are the breaking changes you are expecting from this? Totally fair on the maintenance side, mainly just offered this PR because I'd already done the work. |
Consider for example this playground, i.e. the number of type parameters matters. |
|
Of course, though that just means that existing |
No, you are right. The playground failed due to an unrelated error and adding a defaulted type parameter should work, c.f. https://doc.rust-lang.org/cargo/reference/semver.html#generic-new-default
Personally, I am still firmly against it and would prefer if the tagging/branding is done downstream. But I would like to hear what the other maintainers have to say about it before reaching a conclusion. |
2 participants
Currently, all IDs from a Tree's operations return a concrete
NodeIDstruct, which is a thin wrapper around a non-zero usize that can be created from a usize or have the stored usize retrieved from the object. This works well at enforcing that a user provide aNodeIDto access a node on the tree, but doesn't provide any protections against using the ID from one tree on another tree.With this PR,
NodeIDwill remain the default ID type, but users can create a tree with a customNodeIDusing theTree::new_tagged()method. These IDs will need to implement the newIDtrait, which includes a creation method and an access method (the same methods that NodeID previously had). Users can create their own IDs using the newids!macro, which takes in an ID name and implements all needed methods automatically.This PR has no breaking changes to the user-facing API.
Tree::new()is not generic and will always return a tree which usesNodeIDas its ID. Thetree!macro similarly always creates a tree withNodeIDas its type, but can now optionally take an ID-type parameter as it's first argument to specify the ID type to use.extend_tree,prepend_tree, andappend_treehave all been modified to work with trees which use the same ID type as the current tree, andextending,prepending, andappending with a tree of a different ID type requires the..._transmute_treetags to opt-in to fusing these trees of different type.