Skip to content
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

Change "id" to "$id", retain "id" as deprecated. #154

Closed
wants to merge 1 commit into from

Conversation

handrews
Copy link
Contributor

This addresses issue #20.

"$id" matches "$schema" and "$ref", establishing a clear naming
pattern for all keywords defined in the core specification. This
also reduces confusion in the very common case where objects
described by the schema have an "id" property.

The current plan is to continue to allow for "id" until a migration
tool can be provided.

@handrews
Copy link
Contributor Author

Note: If we end up going in the direction of #155 (which I posted to illustrate the approach- it's not a final proposal) then this PR becomes irrelevant.

@epoberezkin
Copy link
Member

I suggest not including it into v6, see #137 (comment)

@handrews
Copy link
Contributor Author

@epoberezkin having read #160 I see no reason to keep this small (and for v6, backwards-compatible) change out of v6. #160 would be a radical departure whether we have "id" or "$id", so let's start being consistent about core keyword naming ("$schema", "$id", "$ref"). There is already at least one implementation that uses "$id" instead of "id", and at least one other that can't handle instance properties called "id" because of the naming overlap- that's a bug, but indicative of the problems of the current name.

Plus, we already managed to get broad agreement on this rename.

Copy link
Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

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

Eyeballed all cahnges. Look like a fix for the issue to me

"id": {
"description": "This keyword has been deprecated in favor of \"$id\"",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just drop it?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably a good idea, though the patch does include a comment saying it'll likely get removed in the future.

And of course, just because it's removed doesn't mean that implementations can't still support it (i.e. silently upgrade it).

Lemme think about this some, unless you can elaborate more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have strong feelings on this. The deprecation option came in response to migration concerns, although as you note there are several easy options for handling the change. If y'all decide to drop id entirely I'll be happy to update the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think dropping from spec and optionally supporting in implementations addresses migration issues. I'd rather we keep it simple here, so new implementations don't have to carry this baggage...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@awwright @epoberezkin I've submitted PR #212 as a version of this that drops "id" entirely.

This addresses issue json-schema-org#20.

"$id" matches "$schema" and "$ref", establishing a clear naming
pattern for all keywords defined in the core specification.  This
also reduces confusion in the very common case where objects
described by the schema have an "id" property.

The current plan is to continue to allow for "id" until a migration
tool can be provided.
@handrews
Copy link
Contributor Author

See also #212 for an alternative that does not retain "id".

@handrews
Copy link
Contributor Author

Since @awwright and @epoberezkin have both approved #212, I'm going to close this to reduce noise.
@Relequestual if you end up objecting to #212 I can either re-open this or re-start a new PR.

@handrews handrews closed this Dec 29, 2016
@handrews handrews deleted the id branch January 5, 2017 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants