-
Notifications
You must be signed in to change notification settings - Fork 1
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
relationship_descriptor -> name, RelationshipTarget -> HolonCollection #108
Conversation
…tagedcollection-into-holoncollection fix 110 all tests passing
fixed in #110 |
This PR represents a LOT of progress in important areas of the MAP and, for the most part works well. However, visual inspection of the sweetest results for the final get_all_holons dance shows an empty relationship map for the Book holon. The preceding commit response showed that holon having two entries in its relationship_map (as expected). This suggests a problem either on SAVING the smartlinks OR on building the relationship map FROM the smartlinks. From the trace output on the get_all_holons dance, it looks like holochain is retrieving links. So the most likely source of the error is on the build relationship map side instead of the save_links side. Even with debug level tracing enabled, I'm not seeing the expected debug messages from get_relationship_name_from_smartlink: This suggests the function is not being called. I tried doing a full nom run clean and rebuilding everything, but the test results were the same. It is difficult to verify the result, so the proposal in the issue that we just using visual inspection of test results is probably insufficient. We should enhance debug tracing to provide better visibility into what's happening and we should ALSO enhance our test logic to do automatic verification of test results. There is also bit of minor re-factoring I'm going to recommended. |
ah well, duh! We don't have the get_related_holons query dance implemented until issue #107, so there is no way to exercise the functionality we've implemented here until we have that dance and can add a test step for it to the fixture. Guess that's why I said we couldn't really test this in 105. Please ignore my comments about test results. I want to try a bit of re-factoring on a few functions before merging this PR. But, otherwise, everything looks pretty good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit minor refactoring and cleanup, test and sweetest pass resolved all the problems I identified during the review EXCEPT for one problem that will be resolved in Issue #114.
Confirmed all sweetest cases and tryorama tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be deleted. SmartCollection has been replaced by HolonCollection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be deleted, StagedCollection has been replaced by HolonCollection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_accessible function logic looks correct, but it is a bit of work to translate between the requirements and the code. The "READ" check explicitly checks the collection_state that errors, else returns OK on anything else, whereas the "WRITE" checks for the collection_state that is OK and errors on anything else.
It may be a bit more maintainable and easier to update if requirements change by having both arms use similar logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-implemented in minor refactoring and cleanup, test and sweetest pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The <build_relationship_map_from_links> function:
- Doesn't handle the case of a relationship_name being supplied (call to get_relationship_links always passes None for relationship_name instead of passing the supplied relationship_name).
- The logic inside the for link in links loop should be consolidated into a single function call on a (new) <decode_to_smartlink()> function that creates a SmartLink object from a holochain link.
- Unnecessarily clones the entire Vec every time it wants to push a new reference.
I intend to resolve and commit 1 and 3. I've defined Issue #114 to resolve 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items 1 and 3 resolved in minor refactoring and cleanup, test and sweetest pass.
for issue 105, staged_reference.rs line 81
got me thinking, before proceeding, is an enum for HolonCollection for sure what we want?
if so, what implications are that for the logic in add_related_holons ?
in the new architecture, could a relationship name have both an associated staged and smart collection?
if not, then it looks like this function and ensure_editable_collection would take some significant rewiring. I wanted to checkin before continuing.