-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add event handler #366
base: main
Are you sure you want to change the base?
Add event handler #366
Conversation
davidlehn
commented
Feb 23, 2020
- An event handler option seemed like a good approach for a way to allow custom handling of warnings (to display in UIs or throw errors or whatever).
- I was thinking a "strict mode" handler could be available that would turn warnings into errors.
- The event approach with chainable handlers also turned out to work well for the case of capturing and replaying events for cached processed contexts.
- I imagine other events beyond warnings might be useful.
- Open to ideas for alternate approaches.
@@ -517,9 +526,19 @@ api.createTermDefinition = ({ | |||
} | |||
|
|||
if(reverse.match(KEYWORD_PATTERN)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's an example where this would match? Looks like the above checks wouldn't allow {"@reverse": "@foo"}
or similar to get this far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous checks don't specifically look at @reverse
, except to verify that it's a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It checks it's a string, _expandIris
it, then _isAbsoluteIri
checks that. Seems like a keyword like string wouldn't get beyond those steps. Is there an example where it would?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just created w3c/json-ld-api#389, as the order of checks was inverted.
Some rambling thoughts on this patch:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of this @davidlehn!
I agree with your latest comments ... and I would also ask, any reason why the event handler stuff can't support async? We could add await
code, etc. -- and it would only get called whenever event handlers were actually installed. My understanding is that we made the processing functions async so this should be possible... but I also feel like we discussed this already and you said it couldn't/shouldn't be done for some reason, but I can't recall what the issue was.
Some of the events happen in createTermDefinition. If event handlers were to be async, we'd have to make that async, and callers like _expandIri async, and maybe others up the stack. I was afraid that might have performance effects, but I didn't check. Another case where the in-progress benchmarking code needs to be improved so we can test such changes. I figured for version 1 the handlers could be sync and if we change to async/await in the future, they'll still work. |
- Current use is for custom handling of warnings. - Replay events when using cached contexts. - Flexible chainable handlers. Can use functions, arrays, object code/function map shortcut, or a combination of these. - Use handler for current context warnings.
Codecov Report
@@ Coverage Diff @@
## main #366 +/- ##
==========================================
+ Coverage 92.67% 93.37% +0.70%
==========================================
Files 23 24 +1
Lines 2880 2899 +19
==========================================
+ Hits 2669 2707 +38
+ Misses 211 192 -19
Continue to review full report at Codecov.
|