Skip to content

Add @protected tests and a protectedMode flag. #69

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
wants to merge 3 commits into from

Conversation

davidlehn
Copy link
Contributor

@davidlehn davidlehn commented Mar 9, 2019

If it's decided to change the naming from @sealed to @protected, here's a patch.
(replaces #68)
(builds upon #70)

  • Changes simple naming text as needed.
  • Adds a number of tests.
  • Adds a protectedMode option that can be error or warn.
    • The default error mode will fail on protection violations.
    • There are two new error strings.
    • The warn mode will not fail but will enforce the protected terms.
  • Test both error and warn mode as needed.

A prototype jsonld.js implementation is available.

jsonld.js initially just handled the easier error mode with exceptions. But that was the opposite of what the tests did. The warn mode required some invasive internal passing around of options. But otherwise the @protected changes were not too difficult. (At least once you wrap your head around how the warning mode should work!)

Items todo for this patch:

  • Get feedback on this direction, error names, and test coverage.
  • Update spec text for how to handle protectedMode.
  • Ensure spec text matches what tests are doing.
  • Add error strings to spec.
  • Ensure there is a test showing behavior of Does @sealed consider order? json-ld-syntax#153

Preview | Diff

@gkellogg
Copy link
Member

gkellogg commented Mar 9, 2019

I'm fine with changing @sealed to @protected. I'm not sure sure about adding a new protectedMode flag with behavior to error or warn. Processors, such as my own, may implement their own validation mode to raise errors for different cases (e.g. strict checking of values). Otherwise, we may want to just make it an error, and tacitly allow processors to work in a more forgiving mode. In any case, the WG needs to discuss this, as all we've approved is that a warning should be issued.

I'd suggest separating these concerns and keep this one purely on changing @sealed to @protected, then we can address protectedMode in a separate issue, and consider other things that might be included.

@iherman
Copy link
Member

iherman commented Mar 10, 2019

I'd suggest separating these concerns and keep this one purely on changing @Sealed to @Protected, then we can address protectedMode in a separate issue, and consider other things that might be included.

I am in favor of this.

@davidlehn davidlehn changed the title Change from "sealed" to "protected". Add @protected tests and a protectedMode flag. Mar 12, 2019
@davidlehn
Copy link
Contributor Author

I'm making a mess of PRs, sorry! ;-) I made a new PR for just the first commit of this PR that just does the renaming: #70. Can discuss that part there and the extra bits here.

@davidlehn
Copy link
Contributor Author

I'm in favor of @protected violations causing an error by default. Continuing processing is possible but at that point I'm pretty sure you can/will end up with possible garbage output that didn't match the authors intent? It seems that there are use cases where you may want to process data anyway, hence the idea of a mode flag. I'm unclear if that should be a defined spec mode or just a bonus processor feature.

The spec text as is could be improved. But maybe this is moot if the behavior ends up changing. The current language says that processing is aborted and processors should issue a warning. Does "processing is aborted" mean throw an error and quit? Or just abort that algorithm step and continue? (Based on the tests, you continue.) When it's a warning, what is the behavior? Is that something that should be defined and tested?

Since the tests were written to test against warning mode output, this patch at least tries to improve that test coverage for protected terms and null contexts. Tests were also added for the error cases since I imagine implementors would want something like that. But that does depend on specific error strings and spec text.

Happy to adjust all this as needed.

@gkellogg
Copy link
Member

For w3c/json-ld-syntax#137.

- 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.
@davidlehn davidlehn force-pushed the add-protected-tests branch from 23cd03c to 9cf5468 Compare April 12, 2019 16:12
@gkellogg gkellogg mentioned this pull request Apr 13, 2019
@gkellogg
Copy link
Member

Closed in favor of #78.

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.

3 participants