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

Branching logic issue in Create Term Definition step 16 #241

Closed
kasei opened this issue Dec 11, 2019 · 13 comments
Closed

Branching logic issue in Create Term Definition step 16 #241

kasei opened this issue Dec 11, 2019 · 13 comments

Comments

@kasei
Copy link

@kasei kasei commented Dec 11, 2019

Create Term Definition step 16.1 uses passive language that doesn't seem to have any action expressed:

If value contains the @id entry is null, the term is not used for IRI expansion, but is retained to be able to detect future redefinitions of this term.

If no action is taken as a result of this step, step 16.4 will call IRI Expansion with a null value and raise an Invalid IRI mapping error. I don't believe this is the intention of step 16, and ask that it be clarified.

I believe the intended set of steps is something like this:

16. If value contains the entry @id and its value does not equal term:
    16.1. If value contains the @id entry is null, the term is not used for IRI expansion, but is retained to be able to detect future redefinitions of this term.
    16.2. Otherwise:
        16.2.1: [old 16.2, without "Otherwise" prefix] If the value associated with the @id entry is not a string, an invalid IRI mapping error has been detected and processing is aborted.
        16.2.2: [old 16.3]
        16.2.3: [old 16.4]
        16.2.4: [old 16.5]
        16.2.5: [old 16.6]
@azaroth42 azaroth42 added this to Discuss-Call in JSON-LD Management Dec 11, 2019
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 12, 2019

If the value is null, the remaining sub-steps in 16.1 should be skipped.

@iherman

This comment has been minimized.

Copy link
Member

@iherman iherman commented Dec 13, 2019

This issue was discussed in a meeting.

  • RESOLVED: Add to 16.1 that the following substeps should be skipped if the test for null is true
View the transcript Create Term Definition 16.1 needs more indents
Rob Sanderson: #241
Gregg Kellogg: we added some text in 16.1 to retain the information that @id has value null
Proposed resolution: Add to 16.1 that the following substeps should be skipped (Rob Sanderson)
Gregg Kellogg: in my code, the following substeps are skipped
Proposed resolution: Add to 16.1 that the following substeps should be skipped if the test for null is true (Rob Sanderson)
Gregg Kellogg: +1
Rob Sanderson: +1
Pierre-Antoine Champin: +1
Harold Solbrig: +1
Tim Cole: +1
Ivan Herman: +1
Benjamin Young: +1
David I. Lehn: +1
Resolution #6: Add to 16.1 that the following substeps should be skipped if the test for null is true
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 18, 2019

@kasei, please let us know if changes in #247 satisfactorily address your concern.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 18, 2019

As mentioned in #247, I think the merged change isn't ideal as it is stylistically different from, and less explicit than semantically similar constructs used elsewhere in the API document.

@gkellogg gkellogg moved this from Editorial work complete to Discuss-Call in JSON-LD Management Dec 19, 2019
@iherman

This comment has been minimized.

Copy link
Member

@iherman iherman commented Dec 20, 2019

This issue was discussed in a meeting.

  • RESOLVED: Accept pchampin’s addition in PR 266 to further improve #241
View the transcript 16.1 needs more indents
Rob Sanderson: The last one needs discussion.
Rob Sanderson: #241
Gregg Kellogg: The change we made before was regarding a spec update in 16.1.
… We typically don’t do a change like this.
… We want to do the minimum change possible that addresses the issue, which is not necessarily how we would do it prior to CR.
Rob Sanderson: I’m happy eitherway. The further indentation is clearer and more consistent, but reducing the changes to what is necessary is probably a good operating mode.
Pierre-Antoine Champin: I also made a PR on this issue.
… I propose to not add an indentation, but change the step numbers.
… My proposal merges 16.1 into 16.
Pierre-Antoine Champin: #266
Gregg Kellogg: I need to look into it more carefully, but it looks like a better solution. It sticks with our normal style.
Rob Sanderson: This is the last issue before moving to BP.
Gregg Kellogg: Have we closed all issues that I marked as propose-closing?
Rob Sanderson: Do we want to accept pchampin’s change? Or do you want to read it first gkellogg?
Gregg Kellogg: We can go with pchampin’s change.
Proposed resolution: Accept pchampin’s addition in PR 266 to further improve #241 (Rob Sanderson)
Pierre-Antoine Champin: I anticipated that this PR might be rejected because it is sensitive. The first commit in the PR fixes an obvious typo, so it should definitely be retained, even if the PR is not merged.
Rob Sanderson: +1
Ivan Herman: +1
Gregg Kellogg: This PR is good anyways, but I may nitpick it a bit.
Ruben Taelman: +1
Tim Cole: +1
Pierre-Antoine Champin: +1
Gregg Kellogg: +1
Adam Soroka: +1
Resolution #8: Accept pchampin’s addition in PR 266 to further improve #241
@azaroth42 azaroth42 moved this from Discuss-Call to Editorial Work in JSON-LD Management Dec 20, 2019
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 20, 2019

@kasei please look at the update, with @pchampin's wording. It merges the check in 16.1 up to 16, but otherwise preserves the steps.

@gkellogg gkellogg moved this from Editorial Work to Editorial work complete in JSON-LD Management Dec 21, 2019
@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 22, 2019

@gkellogg I agree with @davidlehn's comment in #266 that the new formatting is confusing. The green note syntax leads to very a very strange visual presentation, and given that the Typographical conventions section indicates that notes have a "Note" header, it's not even clear from the text that the new wording is in fact just a note.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 24, 2019

@kasei please indicate your acceptance on this issue, for the record.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 24, 2019

@gkellogg I'm concerned that the new condition in 16 isn't right. The note is helpful, but I think moving the test for null into the condition of 16 ends up messing up the chain of "otherwise" clauses in 17–20. I think test t0032 gets caught up here; it should hit 16 but do nothing, but instead it now fails 16 and ends up in 20 for the university term.

@gkellogg gkellogg moved this from Editorial work complete to Editorial Work in JSON-LD Management Dec 27, 2019
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 27, 2019

@kasei ended up doing your original proposal to put 16.2-n in an "Otherwise" clause in PR #288.

Please let us know if we're done.

@gkellogg gkellogg moved this from Editorial Work to Editorial work complete in JSON-LD Management Dec 27, 2019
@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 30, 2019

@gkellogg I don't see the change in #288 (I only see a changelog entry). Am I missing something?

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Dec 30, 2019

Changes seem to have been lost in a rebase, I restored them.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Dec 31, 2019

@gkellogg looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.