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

Handling @context in Create Term Definition #621

Open
multimeric opened this issue Nov 30, 2024 · 4 comments
Open

Handling @context in Create Term Definition #621

multimeric opened this issue Nov 30, 2024 · 4 comments

Comments

@multimeric
Copy link
Contributor

multimeric commented Nov 30, 2024

I have some concerns about section 21 of 4.2 Create Term Definition algorithm:

It doesn't make sense to me for a few reasons:

  1. In 21.4 we set the "local context" of a term definition. However a term definition only has a property called "context" and not "local context", as defined in section 4.1 Context Processing Algorithm
  2. The context variable which we pull from the @context key is, at this stage, just a map. It doesn't become a context object until we execute the context processing algorithm on it. However, we throw away the result of this algorithm in 21.3 and instead just set the term definition's context to context in 21.4, which is the "raw" unprocessed version. The same section as above specifies that the type of context for a Term Definition is context, which to me means a processed context, the result of the context processing algorithm.

Therefore I think the fix is to remove the "note" from this section, and instead assign term_definition.context = create_term_definition(context, ...). Happy to put in a PR.

@multimeric multimeric changed the title Handling @context in Create Term Definition #850 Handling @context in Create Term Definition Nov 30, 2024
@gkellogg
Copy link
Member

gkellogg commented Dec 2, 2024

I have some concerns about section 21 of 4.2 Create Term Definition algorithm:

BTW, you can see the rendered editor's draft on GitHub here.

It doesn't make sense to me for a few reasons:

  1. A term definition doesn't have a property called "local context", it just has a property called "context", as defined in section 4.1 Context Processing Algorithm

I believe you're correct, and that it is the context of definition which should be set to context.

  1. The context variable which we pull from the @context variable is, at this stage, just a map. It doesn't become a context object until we execute the context processing algorithm on it. However, we throw away the result of this algorithm and instead just set the term definition's context to context, which is the "raw" unprocessed version. The same section as above specifies that the type of context for a Term Definition is context, which to me means a processed context, the result of the context processing algorithm.

The purpose of calling the Context Processing algorithm on the extracted context is to validate it; that is why the un-processed version (array, string, or map) is set in the term definition and not the processed context. It can't be fully processed until it's used in the proper scope (that's why it's a 'scoped context". Then it's processed , it will be combined on top of any existing context state, which could be different each time it's used.

Therefore I think the fix is to remove the "note" from this section, and instead assign term_definition.context = create_term_definition(context, ...). Happy to put in a PR, I just can't find the part of the repo where the latest spec is actually built from.

So, this would not be valid given the way the algorithms are intended to use scoped contexts.

@multimeric
Copy link
Contributor Author

multimeric commented Dec 2, 2024

Thanks, I take your point about the scoping which I admit I don't fully understand. Can you comment on the issue of type checking though? How can we assign a context to be a JSON map when it's defined in the spec as a context object? This actually breaks my code because I defined separate types for e.g.active_context: Context (a custom class) and local_context: Dict[str, Json] (where Json is any JSON object). So when we assign definition.context = context, the type checker rightly flags this as a type error.

@gkellogg
Copy link
Member

gkellogg commented Dec 3, 2024

The spec says that a term definition consists of several entries including an optional context. Context is defined as a set of rules for interpreting a JSON-LD document, which I think is consistent with the unprocessed value of @context. A context definition would be the processed form of a context.

@multimeric
Copy link
Contributor Author

multimeric commented Dec 3, 2024

Thanks, in that case I will interpret the spec as meaning that "a term definition has a field called context which is a local, unprocessed JSON map", or in Python code:

class TermDefinition:
    context: dict[str, JSON]
    # Other fields omitted

My confusion is part of a greater issue about the ambiguity of the data type of context objects in the spec. In general when it says "local context" in means an unprocessed map, and when it says "active context" in means a processed context (what you call a context definition). However this is not consistent, as demonstrated by a term definition having a field called context which is an unprocessed map. I think this could be improved by:

  • Being more consistent with the terms "local context" and "active context"
  • Labelling the type for context variables explicitly, and/or
  • Using the term "context definition" when "processed context definition" is intended

I suppose this could be considered a separate issue to my original concern, so I'm happy to file it separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Errata
Development

No branches or pull requests

2 participants