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

[FEATURE]: JID #1473

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[FEATURE]: JID #1473

wants to merge 3 commits into from

Conversation

amadolid
Copy link
Collaborator

No description provided.

@amadolid amadolid force-pushed the feature/jid branch 11 times, most recently from ca8aaf4 to 38f4752 Compare November 29, 2024 19:47
@amadolid
Copy link
Collaborator Author

amadolid commented Dec 2, 2024

@amadolid amadolid force-pushed the feature/jid branch 5 times, most recently from 8707b96 to cda5059 Compare December 3, 2024 15:36
@amadolid amadolid marked this pull request as ready for review December 3, 2024 16:00
Copy link
Contributor

@ypkang ypkang left a comment

Choose a reason for hiding this comment

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

Is this ready for review and merge @amadolid? I went through it and i think there are a couple of things we need to discuss.

@@ -67,15 +66,15 @@ walker allow_other_root_access {
if self.via_all {
Jac.unrestrict(here, self.level);
} else {
Jac.allow_root(here, UUID(self.root_id), self.level);
Jac.allow_root(here, JacLangJID(self.root_id), self.level);
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if we can somehow not having separate handling for jid between jaclang and jac-cloud. It would be cool if the jac code is the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

for example, we define a plugin method that converts string to the corresponding JID type

TARCH = TypeVar("TARCH", bound="Architype")
TANCH = TypeVar("TANCH", bound="Anchor")

JID_REGEX = compile(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a docstring here with an example jid.

class Anchor:
"""Object Anchor."""

architype: Architype
id: UUID = field(default_factory=uuid4)
root: Optional[UUID] = None
id: Any = field(default_factory=uuid4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be more specific on type here and avoid Any?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, the ID could be anything as it's also technically a read only field



@dataclass(eq=False, repr=False, kw_only=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we keep repr=False? since we are defining a custom repr

Copy link
Collaborator Author

@amadolid amadolid Jan 7, 2025

Choose a reason for hiding this comment

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

If I remember correctly, it will be only generated if you haven't specified __repr__

source: NodeAnchor
target: NodeAnchor
source: JID["NodeAnchor"]
target: JID["NodeAnchor"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the reference now is only the ID instead of actual NodeAnchor

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