Protected term errors #78
Conversation
|
Pending any comments to the contrary, I'll plan to merge this tomorrow. |
| and <var>active context</var> has an existing | ||
| <a>term definition</a> for <var>term</var> which is protected, | ||
| a <a data-link-for="JsonLdErrorCode">protected term redefinition</a> error has been detected, | ||
| and processing is aborted.</li> |
There was a problem hiding this comment.
The one thing we might want to modify here is to allow for "redefinitions" that cause the exact same term definition to occur. There's really no reason disallow that from an authorship perspective -- and people might be surprised that they can't combine two contexts that share a common base context with protected terms. On the other hand, this would complicate implementation.
@davidlehn -- since you were in the code most recently, would this be a tough thing to support?
There was a problem hiding this comment.
Either way, I don't think that this should hold up the PR.
There was a problem hiding this comment.
We could say that an attempt to redefine a term definition that is exactly equivalent to the existing definition is allowed, needing to define what we mean by equivalent, but it seems an unnecessary complication to me.
There was a problem hiding this comment.
Looking good.
In the Compaction algorithm, 'from term' is never set to true when invoking the Context Processing Algorithm (even when it processes a property-scoped context). I'm guessing you know what you are doing here, but I'm pointing it out just in case...
It's passed as |
|
@pchampin, Oh you said in the compaction algorithm ... I'll double check with @davidlehn to make sure we didn't need something there. I suspect we might want another test. |
|
You’re right, it’s unused in the Compaction algorithm. The test is used when expanding, but perhaps there’s an unconsidered case when compacting with what would have been an illegal context. This could use some tests. I’ll take another look. |
- Check for errors when `@protected` conditions are violated.
- Simplify text in older tests. - Support 'protectedMode' option of 'error' or 'warn'. - Test both 'protectedMode' types where needed. - Using 'e' or 'w' suffix for test id but same input file. - Add more tests of various situations.
…itions unless from a term used as a property which has a scoped context. Remove protected term tests with warning option.
…_active property_ has a _local context_. Add tests to be sure attempts to change a context with protected terms fail, unless from a scoped context used for a property.
gkellogg
force-pushed
the
protected-term-errors
branch
from
cec9e5d to
8b0e774
Compare
There was a problem hiding this comment.
Thanks, @gkellogg! Sorry we didn't get to adding these today, we were swamped with other work. We'll check them against our JS implementation as soon as we can. Just one note -- you added 4 tests (which all look correct to me) but only 3 made it into the manifest. Otherwise, this looks great.
Edit problem, added in again. |
|
Thanks again, @gkellogg. Our JS implementation passes all these tests now. I think the create term definition algorithm needs a tweak though (hard to tell w/o preview mode) -- wherever it says to call the algorithm recursively it needs to pass the options (so if we're processing a property term scoped context that flag gets propagated during recursion). |
It doesn’t seem to be necessary to propagate the flame recursively from Create Term Definition. Do you see a case where this affects processing? |
Hmm ... I think you're right. Looking at the algorithm text it we only recurse when there's a prefix that needs to be defined and under those conditions there are no potential Ok, I think we're good to go here. |
Replaces #69.