Skip to content

Conversation

gkellogg
Copy link
Collaborator

@gkellogg gkellogg commented Dec 31, 2019

Code added for framing, but not enabled due to big backlog on framing in general.

@gkellogg
Copy link
Collaborator Author

gkellogg commented Jan 3, 2020

Not directly related, but it touched on some of the same code, so I added some support for keyword oddities.

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @gkellogg! This mostly looks ok to me ... I'm not sure we want console.warn in there (does this break some environments, @davidlehn?).

@@ -350,6 +350,11 @@ api.createTermDefinition = ({
'Invalid JSON-LD syntax; keywords cannot be overridden.',
'jsonld.SyntaxError',
{code: 'keyword redefinition', context: localCtx, term});
} else if(term.match(/@[a-zA-Z]+$/)) {
// FIXME: remove logging and use a handler
console.warn('WARNING: terms beginning with "@" are reserved' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using console.warn here is the same issue that resulted in the protectedMode warn/error flag. That was supposed to eventually change a better handler, but that hasn't been designed yet. Might want another similar flag and we can fix them all at once. The machinery to pass around those options still exists so would be easy to add. I'm not sure what the best behavior even is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this for you to come up with a comprehensive solution. There are other areas where the spec says to generate warnings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Printing out warnings to a console can be invisible for backend code and UIs, hence the idea to have some of handler. I haven't put time into a design for that. A more strict mode that throws errors seems useful to me as well.

@gkellogg gkellogg requested a review from davidlehn January 6, 2020 19:58
@gkellogg gkellogg requested a review from davidlehn January 6, 2020 20:13
@davidlehn davidlehn added this to the JSON-LD 1.1 milestone Jan 6, 2020
@gkellogg gkellogg force-pushed the included branch 2 times, most recently from 4018bf0 to 26c6884 Compare January 25, 2020 23:42
@davidlehn davidlehn deleted the included branch January 29, 2020 00:34
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