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

feat: Create links from/to ExternalHash #380

Merged

Conversation

c12i
Copy link
Collaborator

@c12i c12i commented Sep 24, 2024

Closes #254
Closes #389

@c12i c12i force-pushed the feat/add-option-to-create-link-with-externalhash branch from 3231d79 to 5d11459 Compare September 30, 2024 14:23
@c12i c12i requested a review from matthme September 30, 2024 15:15
@c12i c12i marked this pull request as ready for review October 9, 2024 09:30
@c12i c12i added the ShouldBackport/0.4 This change should be backported to develop-0.1 label Oct 9, 2024
src/error.rs Outdated Show resolved Hide resolved
@matthme
Copy link
Collaborator

matthme commented Oct 13, 2024

I noticed that the [None] option provided by get_or_choose_optional_reference does not make sense to me (I see that this was already there before this PR). Here, why would a link only optionally point to a target? The actual behavior now when a link-type is scaffolded with target [None] is that the target is the same as the base which seems not useful in any real case I can think of. It seems to me that for link-types, both base and target should always be non-optional.

And as another remark, I think it might be worth adding ExternalHash as an explicit option as well. AnyLinkable hash is the more generic type which entails ActionHash, EntryHash and ExternalHash but sometimes it might be desirable to be explicit that one wants to point to an ExternalHash.

@matthme
Copy link
Collaborator

matthme commented Oct 13, 2024

It might even make sense to replace AnyLinkableHash with ExternalHash since if all ActionHash, EntryHash, AgentPubKey and ExternalHash are available as options then AnyLinkableHash is covered implicitly as well.

@matthme
Copy link
Collaborator

matthme commented Oct 13, 2024

It might even make sense to replace AnyLinkableHash with ExternalHash

I'm not sure though...maybe there are cases where this flexibility is desired, i.e. that you'd want to allow for a link that can point to either of these types and is not hard-coded. So keeping AnyLinkableHash as an option as well (alongside ExternalHash and the others) might not hurt.

@c12i
Copy link
Collaborator Author

c12i commented Oct 14, 2024

I noticed that the [None] option provided by get_or_choose_optional_reference does not make sense to me (I see that this was already there before this PR)

Yes this [None] option has always existed, it's exact usecase is actually not documented, perhaps @guillemcordoba can shed some light. Decided to keep it to not potentially break anything that might depend on it.

why would a link only optionally point to a target?

From my understanding of the previous code, I think the term optional doesn't really communicate the purpose correctly. I see that the optional check just adds more options to the default list of all entries that can have links created to it. The difference between get_or_choose_referenceable and get_or_choose_optional_reference_type fundamentally ends up being that the latter contains extra options i.e None and ExternalHash that are not provided by get_or_choose_referenceable to make sure that these two options are only assignable to the "to referenceable"

@c12i
Copy link
Collaborator Author

c12i commented Oct 14, 2024

It might even make sense to replace AnyLinkableHash with ExternalHash since if all ActionHash, EntryHash, AgentPubKey and ExternalHash are available as options then AnyLinkableHash is covered implicitly as well.

I was actually torn between these, but it makes sense to be explicit in this case.

@c12i c12i changed the title feat: Create links from/to AnyLinkableHash feat: Create links from/to ExternalHash Oct 14, 2024
@guillemcordoba
Copy link
Collaborator

The [None] option is to be able to support "metadata" only links. In holochain, the only way to attach a small piece of extra information to a hash is creating a link from that hash and add the information as the tag. In this case the target is actually irrelevant, because the only point of the link is to attach is link tag to the hash. This is the use case I wanted to support with the None option.

You might argue that this use case could be done by targeting any other entry hash. But then the scaffolding tool would scaffold validation to make sure that the target entry exists, which is not what I want if I only want to attach metadata to that hash.

Hope this helps!

@matthme
Copy link
Collaborator

matthme commented Oct 14, 2024

Oh interesting, yes this helps. Maybe this could be described a bit more clearly in the option then. Something like [None] (use this link to attach meta-data only) or something.

@c12i c12i force-pushed the feat/add-option-to-create-link-with-externalhash branch from 6d3c514 to b534735 Compare October 15, 2024 08:06
@c12i c12i requested a review from matthme October 15, 2024 08:08
Copy link
Collaborator

@matthme matthme left a comment

Choose a reason for hiding this comment

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

I think it looks good to me know. I did notice some things that I think should be addressed at some point but they can be addressed independently of this PR:

  • if you create a link type, the file name and function names have the target link in plural (I think this is because it uses the same naming logic as for collections where an anchor points to multiple entries). For example validate_create_link_book_to_pages instead of validate_create_link_book_to_page

  • There is a general problem it seems that files can get overwritten in case of name collisions. This can happen most easily with meta-data-only links whose file names are single word names, e.g. book.rs if the link type is named book. So in this case it would overwrite an existing book.rs file from a scaffolded book entry-type. Or the other way around, whichever is scaffolded first.

@c12i
Copy link
Collaborator Author

c12i commented Oct 17, 2024

Yes, I noticed an issue with the naming, will address it separately

@c12i c12i merged commit b4e80cd into holochain:develop Oct 17, 2024
8 checks passed
@c12i c12i removed the ShouldBackport/0.4 This change should be backported to develop-0.1 label Oct 17, 2024
c12i added a commit to c12i/scaffolding that referenced this pull request Oct 17, 2024
* add ExternalHash field types and option to target this type when scaffolding a link-type

* ensure external hash is imported in type declarations

* feat: create links from/to AnyLinkableHash

* Add AnyLinkablehHash field type templates and add to reserved words

* fix failed to resolve errors

* improve checking for reserved keywords

* fix invalid link-type delete method in test template

* simplify reserved_words hashsets

* update cli

* Extend reserved keywords to check for javascript keywords

* Update AnyLinkableHash sample value

* Extend reserved words check tests

* fix AnyLinkableHash link-type tests

* Fix AnyLinkableHash link-type tests and remove redundant AND/OR hbs
helpers

* update inner_choose_referenceable

* /AnyLinkableHash/ExternalHash

* Update invalid serserved word error message

* Refactor entry/link type utils

* Add some context to the [None] option when scaffolding a link-type

* /AnyLinkableHash/ExternalHash in link-type template

* Fix option placement

* Prevent UI from getting generated where the base type of a link is an
ExternalHash

* ExternalHash links can be bidirectional

* Only skip ui if to_referenceable is some and the field_type is of
ExternalHash

* Remove unnecessary into call in delete link function

* Fix rustfmt ci failure

* Fix missing conversion

* Fix react link-type template
c12i added a commit that referenced this pull request Oct 21, 2024
* Bump crate version

* feat: Create links from/to `ExternalHash` (#380)

* add ExternalHash field types and option to target this type when scaffolding a link-type

* ensure external hash is imported in type declarations

* feat: create links from/to AnyLinkableHash

* Add AnyLinkablehHash field type templates and add to reserved words

* fix failed to resolve errors

* improve checking for reserved keywords

* fix invalid link-type delete method in test template

* simplify reserved_words hashsets

* update cli

* Extend reserved keywords to check for javascript keywords

* Update AnyLinkableHash sample value

* Extend reserved words check tests

* fix AnyLinkableHash link-type tests

* Fix AnyLinkableHash link-type tests and remove redundant AND/OR hbs
helpers

* update inner_choose_referenceable

* /AnyLinkableHash/ExternalHash

* Update invalid serserved word error message

* Refactor entry/link type utils

* Add some context to the [None] option when scaffolding a link-type

* /AnyLinkableHash/ExternalHash in link-type template

* Fix option placement

* Prevent UI from getting generated where the base type of a link is an
ExternalHash

* ExternalHash links can be bidirectional

* Only skip ui if to_referenceable is some and the field_type is of
ExternalHash

* Remove unnecessary into call in delete link function

* Fix rustfmt ci failure

* Fix missing conversion

* Fix react link-type template

* feat: Improve `hc-scaffold entry-type` developer experience (#383)

* refactor enum type selection; add option to restart field type
selection; refactor FieldType parsing

* Improve EntryTypeReference parsing

* Improve parse_enum parsing logic

* Update field_type parsing logic

* fix: Optimize nix flake (#384)

* optimize nix flake

* Supress clippy warnings

* Nix flake update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants