Add support for multiple links with the same rel #12
Conversation
| if (m) acc[m[1]] = m[2]; | ||
| if (m) { | ||
| acc[m[1]] = m[2]; | ||
| } |
There was a problem hiding this comment.
Why should this be a one liner and line 14 isn't flagged similarly? Should I change line 14 as well?
There was a problem hiding this comment.
line 14 is fine for two reasons:
- it's a nested if, so you could argue you'd want curlys like the outer if
- and more importantly it didn't just change already existing code for no good reason ;)
There was a problem hiding this comment.
The reason I made the change in the first place was only for consistency. It seemed like all of the other if statements used braces and this one was the exception.
| acc[rel] = xtend(x, { rel: rel }); | ||
| if (acc[rel]) { | ||
| if (!Array.isArray(acc[rel])) { | ||
| acc[rel] = [ acc[rel] ]; |
|
Sorry for the delay, but LGTM in general. |
This incorporates the feedback on the pull request.
|
Updated. |
|
@fidian made you collaborator. Please merge to master following this guide. Thanks. |
|
I appreciate the gesture, but I would rather not become a collaborator on this project unless I would also be able to push to npm. It does not alleviate the bottleneck in the process by adding me as a collaborator. People still rely on the push to npm and that's something I won't be able to do. So, thanks for the offer but I would prefer for you to handle the merge. |
|
Hey just asked for a bit of help here. Either way please merge if you need this and want to help others as well. |
|
It might be my ignorance, but it seems as though this back and forth cost you more time than performing the merge. I do not understand the stance you are taking against merging. What acceptable options do you propose so we can move forward? Here's what I see:
The argument that this discussion is taking more time than simply merging could also be applied to me. I can not merge this myself because I didn't accept the invitation and don't have the desire to be a maintainer on this project for reasons explained in my last posting. I would happily take the project off your hands and move it to tests-always-included to alleviate the burden you have. The group of people there could maintain it. However, the code would probably be changed to conform to the style guide and I feel those changes wouldn't be welcomed. Alternately, perhaps there's people at your work that already use your software and would help shoulder your onerous tasks that are associated with being the project owner. I did require this functionality so I am using my own fork of your repository, so I simply don't use the version in It is unfair to offload project maintenance on anyone unless there was an agreement where the responsibilities were accepted by that person. |
|
Would you please reply to the questions I asked? |
|
I must give up on getting this pull request merged and I won't be following up. The projects I have that use this module will be switched to use http-link-header, which parses multiple links in a single header, helps you query the links, then also converts link definition objects into link headers. It already does what I currently need and the extra bits that I desire for conversion back into a header. I would strongly suggest others switch as well because it converts links in both directions and because it has a more active maintainer. |
|
All I asked is to help me out a bit here and instead you launch into a philosophical discussion. If you wanna help, fine go ahead merge to master an I'll publish. If you don't and want to use the other project instead that's fine with me as well. |
Maintains backwards compatibility as long as there's only one link for a given rel.
is parsed into
{ "item": [ { "href": "http://example.com/one", "rel": "item" }, { "href": "http://example.com/two", "rel": "item" } ], "up": { "href": "http://example.com/", "rel": "up" } }This closes #11.
Edit: Fixing up JSON. That's what I get for writing it by hand.