Skip to content

No default value defined for base URL in Create Term Definition #375

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

Closed
kasei opened this issue Feb 14, 2020 · 12 comments · Fixed by #377
Closed

No default value defined for base URL in Create Term Definition #375

kasei opened this issue Feb 14, 2020 · 12 comments · Fixed by #377

Comments

@kasei
Copy link
Contributor

kasei commented Feb 14, 2020

(Related to #265, #356)

Create Term Definition says that base URL is an optional input variable, but does not define a default value. It is then used unconditionally in step 21.3 and 21.4.

@gkellogg
Copy link
Member

I'll check, but I believe that the only times that base URL would be used in Create Term Definition it is passed as an option.

@kasei
Copy link
Contributor Author

kasei commented Feb 15, 2020

Then I'd suggest adding language for a default value of null.

gkellogg added a commit that referenced this issue Feb 15, 2020
…onLdOptions/base`, which now defaults to `null`.

Fixes #375.
@gkellogg
Copy link
Member

Updated in #377.

@kasei
Copy link
Contributor Author

kasei commented Feb 15, 2020

@gkellogg I don't see any change in #377 to the text about default values for the optional arguments in Create Term Definition. Am I missing something?

@gkellogg
Copy link
Member

See the JsonLdOptions dictionary definition for initializers.

@kasei
Copy link
Contributor Author

kasei commented Feb 15, 2020

How is that relevant? This is about a named, optional argument to Create Term Definition, not about a field in a JsonLdOptions structure. Right?

@gkellogg
Copy link
Member

Step 4 of expand() uses documentUrl, if it is available, otherwise the base option from options. As described for that parameter, default values (e.g., for base) are defined in JsonLdOptions.

It should be the case that when the API calls the Context Processing algorithm that the base IRI and original base URL options will always be set from one of these. There's code in the Context Processing algorithm to check if both context and base IRI aren't valid IRIs.

@gkellogg gkellogg self-assigned this Feb 16, 2020
@kasei
Copy link
Contributor Author

kasei commented Feb 17, 2020

I'm lost. I still see Create Term Definition saying base URL is optional, and not providing a default value. And then using that value unconditionally. expand() is not the only place that Create Term Definition is invoked, so I don't think changes there can resolve this issue entirely.

For example, IRI Expansion 6.3 invokes Create Term Definition without a base URL argument. Now, it may be possible that this invocation cannot reach Create Term Definition 21.3, but that's not obvious to me, which is why I'm concerned about such use of an undefined value.

@gkellogg
Copy link
Member

So, Create Term Definition is always called with the base URL argument. In Step 5 of expand() it is called with original base URL from active context established in step 4. In Step 6, it is called with the contextUrl from the header, which per HTTP will be an absolute URL.

In Step 4, original base URL is established either from the documentUrl of the remote document, or the base option, which is initialized to null in JsonLdOptions, if not set otherwise.

The Create Term Definition algorithm says that the default for base URL is false, but it should be null. I'll update that. But, those calls should probably use the base URL definition from the associated term in the context which is being passed.

@kasei
Copy link
Contributor Author

kasei commented Feb 17, 2020

So, Create Term Definition is always called with the base URL argument.

This is not true. Please see the example I just included above.

The optional inputs are base URL, protected which defaults to false, and override protected, defaulting to false, which is used to allow changes to protected terms..

I did not read this as providing a default value for base URL (since there are three arguments discussed, and two default values).

@gkellogg
Copy link
Member

Yes, I misread that; it does now. It's also not necessary to pass a base URL from those places in Create Term Definition, as the default null value will suffice. (There could be a corner-case when expanding @type values when creating terms, if those @type values have embedded contexts, but those will be picked up when going back to the original term definition.)

gkellogg added a commit that referenced this issue Feb 18, 2020
…onLdOptions/base`, which now defaults to `null`.

Fixes #375.
@gkellogg gkellogg reopened this Feb 18, 2020
@kasei
Copy link
Contributor Author

kasei commented Feb 19, 2020

d78d8d2 looks good. Thanks.

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

Successfully merging a pull request may close this issue.

2 participants