implemented _links property to hold link and relationship links while…

[amwmedia]

Here's a possible implementation for _links. There may be a better way to do this, but this was the most obvious solution I could think of that doesn't break backwards compatibility by turning the relationships array into an array of objects (or something like that).

[beauby]

Hi @amwmedia, thanks for your work (again ;)). A few remarks I'd like to hear your thoughts about:

1. Could you add some tests to ensure that resource-level and relationship-level links are parsed as we expect, and also a test for what happens when no link is provided (I believe with your current patch the _links attribute would not be defined, but it might be better to set it to {} by default).
2. What happens if both a resource and one of its relationships have a self link? I believe currently it would lead to the resource's self link being overwritten. Maybe a solution would be to have a separate _relationship_links property.
3. There should be no need to name the property _links rather than simply links as the spec formally disallows attributes named links. edit: however, seeing that we already have an _attributes property, it makes sense to keep it as _links.

[amwmedia]

Hey @beauby, I didn't want to finalize this too much (with tests and such) before I got your take on the actual implementation. Currently _links is created, and relationships links fall into a _links.relationshipName object. I think this works so long as the spec does not allow for relationships within relationships (which I don't believe is the case). The only possible conflict with this approach would be if you have a relationship named "self" or something unlikely like that.

We could prefix a relationship link property with _ as well. so you'd end up with _links._relationshipName

Thoughts?

[kulor]

hey, @amwmedia thanks for this, I'm keen to use the links functionality so am willing to help out with this if you need it (write tests etc).

I've just checked it out locally and it seemingly works with my heavily linked dataset.

on the discussion about _links vs. links, I vote to stay with _links as it's the internals of the model than the first class attributes.

cc/ @beauby

[beauby]

Hi there! Sorry I took so long (again) to get back. So, after giving it a thought, I definitely agree that _links is more consistent than links. The only thing I'm not comfortable with in this PR is merging the resource and relationship links. I'd much rather have a _relationship_links object storing links for each relationship. Other than that, I'm totally 👍 on this PR and would love to get it merged with some tests!

[amwmedia]

the possibility of a relationship link collision was my only reservation on this as well. I can try to get this change in soon. We are closing out a milestone today at work, so possibly early next week.

[beauby]

Awesome @amwmedia!

[beauby]

Hi @amwmedia, any news on this front?

[amwmedia]

Sorry, things did not slow down as I had hoped after that week. In fact, they got more busy :-). This is still on my radar, but if anyone else wants to make this last small change, feel free. I've just been swamped lately.

[vjustov]

Hey @amwmedia, Did you get a chance to check this out?