Skip to content

Conversation

@jviotti
Copy link
Member

@jviotti jviotti commented Jun 6, 2025

I've been trying to finally incorporate the annotation test suite in Blaze, and this is the one issue I found. Given the schema declares $id, the assertion should probably assert against the actual full URI?

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti jviotti requested a review from a team as a code owner June 6, 2025 16:07
@jviotti jviotti requested a review from jdesrosiers June 6, 2025 16:07
@jviotti
Copy link
Member Author

jviotti commented Jun 6, 2025

Else maybe we can remove the $id?

jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 6, 2025
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 6, 2025
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Yes, that would be the correct expected property, but ...

Else maybe we can remove the $id?

Yes, the $id shouldn't be included. I included that originally because I was returning the schema location as the annotation value and the URI of the schema was relevant to that value. But then we decided in a discussion not too long ago that it should be the schema object and not the location. When I changed the test, the $id was no longer necessary and should have been removed. Although both fixes work, I think the best solution is to remove $id. It's not needed.

@jviotti jviotti changed the title Take into account $id for annotation schema location assertions Remove instances of $id in content related annotation tests Jun 6, 2025
@jviotti
Copy link
Member Author

jviotti commented Jun 6, 2025

@jdesrosiers Makes sense. Done! I updated the PR to remove the identifiers instead

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti jviotti force-pushed the annotations-content-id branch from 1c331cf to 72c670b Compare June 6, 2025 17:54
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 6, 2025
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 6, 2025
@jviotti
Copy link
Member Author

jviotti commented Jun 10, 2025

Is there anything preventing the merge of this PR?

@jdesrosiers jdesrosiers merged commit 48461fc into json-schema-org:main Jun 11, 2025
3 checks passed
@jviotti jviotti deleted the annotations-content-id branch June 11, 2025 11:21
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 11, 2025
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 11, 2025
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 11, 2025
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 11, 2025
jviotti added a commit to sourcemeta/blaze that referenced this pull request Jun 11, 2025
syedazeez337 pushed a commit to syedazeez337/blaze-profiling-lab that referenced this pull request Oct 27, 2025
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.

2 participants