8000 unify id and ids by tkellen · Pull Request #392 · json-api/json-api · GitHub
[go: up one dir, main page]
Skip to content

unify id and ids #392

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 4, 2015
Merged

unify id and ids #392

merged 1 commit into from
Mar 4, 2015

Conversation

tkellen
Copy link
Member
@tkellen tkellen commented Mar 4, 2015

This distinction is unnecessary. Checking if id is a string, an
array or null is trivial for clients.

unify id and ids
31400b6 
This distinction is unnecessary. Checking if `id` is a string, an
array or null is trivial for clients.
@steveklabnik
Copy link
Contributor

I am 👍 on this. Seems like a very minor, odd case.

@bintoro
Copy link
Contributor
bintoro commented Mar 4, 2015

Back in the day people hoped that the keys could be unified, but for unknown reasons the idea didn't make its way into the spec.

Just a couple of days ago, @ethanresnick brought this up again in #312. It's not just about id and ids but the data member too (for heterogeneous relationships). I'm also strongly in favor of unifying all three. Not only is it way more elegant, it's simpler from a parsing point of view, too.

I've investigated the old threads, and it appears there was disagreement on singular vs. plural for the key, and that may even be why it all fell apart. Based on this observation, I suggested in #312 the following:

[...] let's simply define a reference as one of:

  • an ID,
  • an array of IDs, or
  • an array of (ID, type) tuples.

Clearly, a reference could be labeled ref. No semantic controversy there.

I don't insist on any specific key, but I would like there to be just one. If it's an array, you know it's to-many, and if the array elements are objects, you know it's heterogeneous (the same can also be inferred from the absence of a shared type member). Separate keys are totally redundant.

@ethanresnick
Copy link
Member

Yeah, this discussion is happening in #312 too.

I think we should use a new word rather than just chopping off the s, since id almost always denotes a singular id outside of this spec. I'm not a huge fan of ref either, because it seems more cryptic than the other keys in the spec—the only other key that's an abbreviation is id, and that's an abbreviation that's so familiar to most programmers as to not even sound like an abbreviation anymore.

What do you guys think of "target", which, as I proposed in #312, could be used to replace the "data" member too. So the three cases of link objects would look like this.

"links": {
      "employer": {
        "type": "companies",
        "target": "4"
      },
      "skills": {
        "type": "skills",
        "target": ["1", "2", "3"]
      },
      "favorites": {
        "target": [
          {"type": "posts", "id": "4"}, 
          {"type": "people", "id": "2"}
        ]
      }
}

@ethanresnick ethanresnick mentioned this pull request Mar 4, 2015
Closed
@bintoro
Copy link
Contributor
bintoro commented Mar 4, 2015

The reason I like "reference" is that it's perhaps the only word whose actual meaning fits the use case extremely well. It should also be acceptable to those who are concerned about grammatical number.

If the spec wants to avoid abbreviations, then I can appreciate that. But it didn't even occur to me that ref would be cryptic to anyone using JSON API.

target is definitely a possibility. It's just that, in typical usage, the target of a relationship is the whole entity being targeted, generally not just the ID. Does this matter? I could live with it. And target works very well in the heterogeneous case.

One more option that occurs to me is key. It's succinct without being an abbreviation, makes sense semantically, is instantly understandable, and might be justifiable to the singular/plural crowd as well:

"links": {
      "employer": {
        "type": "companies",
        "key": "4"
      },
      "skills": {
        "type": "skills",
        "key": ["1", "2", "3"]
      },
      "favorites": {
        "key": [
          {"type": "posts", "id": "4"}, 
          {"type": "people", "id": "2"}
        ]
      }
}

EDIT: I guess the thing that bugs me about target is that it's not conceptually parallel to id. The link object itself represents the target, while an ID is just one way of referring to it. The member whose meaning is closest to "target" would probably be the resource URL.

EDIT 2: Let's also consider what an empty to-many would look like:

"target": [] — Good ("no targets")
"ref": [] — Good ("no references")
"id": [] — Not that good
"key": [] — Seems the worst of all

steveklabnik added a commit that referenced this pull request Mar 4, 2015
@steveklabnik
Merge pull request #392 from tkellen/unify-id-ids
3e55d97 
unify id and ids
@steveklabnik steveklabnik merged commit 3e55d97 into json-api:gh-pages Mar 4, 2015
tkellen pushed a commit to endpoints/endpoints that referenced this pull request Mar 4, 2015
unify id and ids
55eea5f 
ref json-api/json-api#392
@tkellen tkellen mentioned this pull request Mar 4, 2015
Merged
tkellen pushed a commit to endpoints/endpoints that referenced this pull request Mar 4, 2015
unify id and ids
d3715a5 
ref json-api/json-api#392
@bintoro
Copy link
Contributor
bintoro commented Mar 5, 2015

@steveklabnik, does the merging of this PR indicate that you would be opposed to fusing the third overlapping member (data) into the id/ids combo, or that it should be discussed in a new issue/PR?

@hhware hhware mentioned this pull request Mar 16, 2015
Open
@bmac bmac mentioned this pull request Mar 20, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tkellen @steveklabnik @bintoro @ethanresnick
0