Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
core(jsonld): Structured data validation updates #8137
core(jsonld): Structured data validation updates #8137
Changes from 2 commits
32840b0
85c8f5c
50b8a5e
5e065aa
1275d95
048d1f2
fae6c39
700c4b8
b4b4271
626c77d
d699fa3
a3e00ae
7becbc7
cb7e9cc
e62b7fd
37e9236
1efcefd
545ee2f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
should we put this in quotes or after a colon or something? this also makes we wonder what our i18n story is here 😬
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.
wait a second, the path is backwards? I think we're missing some tests that have paths of length >2 then 🤔
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.
I can look into this more tomorrow, but what makes you think the path is backwards? Here's an example of the path:
[ 'http://schema.org/author', '0', 'http://schema.org/colleague', '0', '@type' ]
For this:
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.
Oh the comment confused me. It says it's getting rid of the "first chunk" but it's removing the last chunk. That made me think it is backwards and all paths started with
/@type
, but I see now :)maybe update the comment? a test for longer paths still couldn't hurt though sometime :)
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.
Oooh, yeah good catch, that's a very confusing comment! Added a test case for deeper nesting too.
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.
these two are utility-y enough, maybe they're worth exposing and having explicit tests for them?
They're getting test coverage right now but I doubt it's much, and it would have the benefit of documenting them a bit more. e.g. these aren't called with literals anywhere, so it's not possible to know what the
path
string generally looks like without also looking at the code that generates paths.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.
Yeah, adding some tests is a good idea 👍
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.
has that shipped sailed with
jsonlint-mod
is there any way to leverage that?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.
also deja vu, I feel like I've reviewed this before so sorry if you've answered this in the previous PR
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.
I don't see a direct way to leverage jsonlint-mod here. It just uses Douglas Crockford's JSON parser.
I guess we could parse it ourselves or find a small parser that we can include?
Yup, I sent you a pre-PR to get some initial feedback.
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.
seems like if we have
currentObj[key]
asundefined
we could just return before this whole loopThere 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.
True, I don't see why we'd need it at all actually...
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.
seems like
keyFound
is unnecessary, we could just return here and throw if we ever make it through the loop without returningThere 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.
we should probably use strict equal here then, these would have been failing before I guess
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.
this name is a bit confusing to me for how it's used. the type isn't invalid, right? it's just the type of the entity that has some other invalid stuff going on?
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.
The entity is not a valid
type
instance because it has a property that doesn't exist ontype
.I originally just called that array
types
, but then it wasn't clear that having that array means invalid. (And we use that array to generate theInvalid Event: unexpected property asdf
message.)Maybe we can call the array
invalidForTypes
?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.
oh gotcha, I actually like the direction you were headed with
.types
then. I think it's clear something is invalid by virtue of the fact that we are giving an error. MaybetypeOfInvalidEntity
or something similar if we're going the more explicit route?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.
I'm at
validTypes
right now, since we only include the types in the validation error if they are valid. (And we don't want to show invalid types in the UI, because in that case they'll be part of the error message.)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.
ok, where is this leading
|
coming from :P Not even prettier does this, I don't thinkThere 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.
my prettier definitely does this in other projects, not sure about here :)
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.
maybe it's a typescript prettier thing? I couldn't get it to repro in their playground