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

Add support for multiple links with the same rel #12

Closed
wants to merge 1 commit into from
Closed

Add support for multiple links with the same rel #12

wants to merge 1 commit into from

Conversation

fidian
Copy link

@fidian fidian commented Jul 27, 2016

Maintains backwards compatibility as long as there's only one link for a given rel.

Link: <http://example.com/one>; rel="item", <http://example.com/two>; rel="item", <http://example.com/>; rel="up"

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.

if (m) acc[m[1]] = m[2];
if (m) {
acc[m[1]] = m[2];
}
Copy link
Owner

Choose a reason for hiding this comment

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

please keep as one liner

Copy link
Author

Choose a reason for hiding this comment

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

Why should this be a one liner and line 14 isn't flagged similarly? Should I change line 14 as well?

Copy link
Owner

@thlorenz thlorenz Nov 15, 2016

Choose a reason for hiding this comment

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

line 14 is fine for two reasons:

  1. it's a nested if, so you could argue you'd want curlys like the outer if
  2. and more importantly it didn't just change already existing code for no good reason ;)

Copy link
Author

Choose a reason for hiding this comment

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

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] ];
Copy link
Owner

Choose a reason for hiding this comment

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

please no spaces after brackets

@thlorenz
Copy link
Owner

thlorenz commented Nov 9, 2016

Sorry for the delay, but LGTM in general.
Please rebase on master and fix the nits I pointed out.

This incorporates the feedback on the pull request.
@fidian
Copy link
Author

fidian commented Nov 15, 2016

Updated.

@thlorenz
Copy link
Owner

@fidian made you collaborator.

Please merge to master following this guide.
I will then version as minor upgrade and publish to npm.

Thanks.

@fidian
Copy link
Author

fidian commented Nov 15, 2016

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.

@thlorenz
Copy link
Owner

thlorenz commented Dec 4, 2016

Hey just asked for a bit of help here.
I've got tons of projects and if you could at least help me to do the merges that makes my life a lot easier. I'd publish to npm pretty much immediately but don't add collabs to that too quick for security reasons ;)

Either way please merge if you need this and want to help others as well.

@fidian
Copy link
Author

fidian commented Dec 5, 2016

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:

  • You merge.
  • You add alternate maintainers to this project and they merge.
  • This pull request stays open and isn't merged.
  • This pull request is closed.
  • The project is moved to an organization that maintains it.

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 npm. I submitted this pull request because I do want to help the community. These words were unwelcome: ".. and want to help others as well." I hope that you were just writing what you were thinking as opposed to provoking action via guilt. I believe those words are more of a "stream of consciousness" as opposed to the implications that I'm selfish or lazy.

It is unfair to offload project maintenance on anyone unless there was an agreement where the responsibilities were accepted by that person.

@fidian
Copy link
Author

fidian commented Jan 30, 2017

Would you please reply to the questions I asked?

@fidian
Copy link
Author

fidian commented Jan 31, 2017

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.

@thlorenz
Copy link
Owner

All I asked is to help me out a bit here and instead you launch into a philosophical discussion.
It's totally normal to not have a project under a different organization and still have active contributors.
I just really don't have the bandwidth to do any of what you asked (nor could I even parse the essay you wrote above in its entirety).

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.

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.

Multiple links for the same link relation do not work.
2 participants