[Hackernews] Show Story Comments

[tapanchudasama]

[shields-ci]

	Messages
:book:	:sparkles: Thanks for your contribution to Shields, @tapanchudasama!

Generated by :no_entry_sign: dangerJS against bf3f27137eb0e0a22fe94068f12100e889428209

[calebcartwright]

Be sure to update the classnames to not have duplicate class registrations (that's why the tests are failing, I presume some leftover copy/pasta)

[tapanchudasama]

Regarding the issue of ID in Karma badge, what should be used instead?

[calebcartwright]

Regarding the issue of ID in Karma badge, what should be used instead?

You mean the label? Ultimately the default just needs be something that ensures the badge's meaning is obvious, folks can always customize as they see fit.

I think consistency with Reddit is probably reasonable, but any of the below should work.

[tapanchudasama]

What if we say U/pg instead of U/pg karma ?

[calebcartwright]

What if we say U/pg instead of U/pg karma ?

No. The badge has to actually include what the metric represents. In the absence of such context, the badge really should be interpreted as meaning there are some X number of the thing on the label, in this case "of those users".

That's a non-starter.

[calebcartwright]

Thanks! Let's pull 0d927cfe4055cc517eceefe88cc6e00efe8fd7d0 out into a separate PR that we can go ahead and land though, because I think there's more to discuss on the item/story related badges

[calebcartwright]

e3b695497a9188c648b11f875883dc7397ca6c34 is the right type of idea, but I think there's actually a lot more we can consolidate. I've admittedly only done a quick pass through, but it seems to me that the code for both is essentially identical beyond the different label text and response field we want to grab.

So in this type of scenario, it would probably be better to create a single class/route that represents metrics/stats of an item/story and we'll have a bounded route parameter, e.g. a route like

  static route = {
    base: 'hackernews/item-',
    pattern: ':variant(comments|score)'
  }

Then within the class we can use the value of the variant parameter to determine what field to grab from the response and what label to set. Note that the label set in defaultBadgeData could be something like item, so the error/not found could display as

I'm still on the fence about the nomenclature regarding item vs. story, given that story is a type of item. Do we know what the other types are (at least as we'll see them in the API response) and what we'd expect when the provided id is some non-story type of item?

[tapanchudasama]

Do we know what the other types are (at least as we'll see them in the API response) and what we'd expect when the provided id is some non-story type of item?

@calebcartwright From the docs: It seems their way of working is that everything is an item that has different properties like score (upvotes), karma, descendants (comments), etc.

[calebcartwright]

It seems their way of working is that everything is an item that has different properties like score (upvotes), karma, descendants (comments), etc.

Yes I realize that, and it's the basis for my question.

We obviously don't have a way of knowing the type of item based on an arbitrary id, but we need to think through how we'd handle other types. E.g., if someone requests a badge from us with an id that is itself, a job, comment, pollopt, what exactly should our behavior be? The same theme applies to the score as well given that the value of that field has different meanings depending on the type.

I.e. it seems wrong to me that if someone gives us an id for a poll that the badge we return is .

I think we could either:

• Have more narrowly scoped routes with error handling (e.g. /hackernews/story-comments/:id and then throw an InvalidResponse error after we get the response from HN if the type is something non-story)
• Keep the route generic and try to add conditional handling to dynamically frame things based on the item type (e.g. vs

[tapanchudasama]

Have more narrowly scoped routes with error handling

I think this is a good way. The code will be less complex, it has better SOC and easy to maintain. WDUT?

[calebcartwright]

Have more narrowly scoped routes with error handling

I think this is a good way. The code will be less complex, it has better SOC and easy to maintain. WDUT?

I don't have enough info yet to have an opinion to be honest. It's probably as reasonable a starting point as any, but I can absolutely envision requests down the road of folks wanting badges for metrics for other item types. Feel free to give it a shot if you'd like though!

[tapanchudasama]

Do you mean to say that badge will have to know if the requested item is story/poll or anything? Shouldn't the user take care of that?

I think we can return whatever the api returns, so whether it is a ID for poll or pollopt or anything, we return the score/comments for that in the badge.

But, this is just my thinking. Not sure if its right way.

[calebcartwright]

Do you mean to say that badge will have to know if the requested item is story/poll or anything? Shouldn't the user take care of that?

I think we can return whatever the api returns, so whether it is a ID for poll or pollopt or anything, we return the score/comments for that in the badge.

But, this is just my thinking. Not sure if its right way.

I'm a bit puzzled by the question given what I thought was agreed to above (narrowly-defined routes with error handling).

If we go that path and have some badge route hackernews/story-comments/:id then that means if a user provides an id that turns out to be an item of a different, non-story type, then the "error handling" piece of this approach means that we should throw an error, e.g. throw new InvalidParameter({ prettyMessage: `item ${id} is not a story` })

We would do this for the reasons I mentioned above in https://github.com/badges/shields/pull/7463#issuecomment-1008320726, because the API response structure is consistent and generic, but the semantics of the values in the response fields are variable depending on the item type. So we either need to dynamically handle that variation, or be static but throw and display an error to the user when an incorrect item type is passed to us as a parameter

[calebcartwright]

Also, will you have the bandwidth(and interest) over the next day or two to address https://github.com/badges/shields/pull/7463#issuecomment-1008032890? That's a separate issue which needs to be fixed ASAP and not coupled to, and queued up behind, the addition of the badges

[tapanchudasama]

Although I am stuck with my day job, I'll spare some time, to fix this.

[tapanchudasama]

I'm a bit puzzled by the question given what I thought was agreed to above (narrowly-defined routes with error handling).

Okay, so my bad. https://github.com/badges/shields/pull/7463#issuecomment-1013694829 Here I am referring to the second option which you proposed here. https://github.com/badges/shields/pull/7463#issuecomment-1008320726

Reason being only because there are not a lot of things which needs to be handled dynamically.

• Score for story/comment/ask/poll/pollopt/job
• comments for story/poll

Thats it. Initially I thought we would need lot many routes, (score/comments/likes/ ..likewise) but it seems I am wrong.

[calebcartwright]

Although I am stuck with my day job, I'll spare some time, to fix this.

No worries, we know how that goes. Thanks for taking care of it :pray:

[PyvesB]

@tapanchudasama this PR and #7462 have been open for a while, are you planning on circling back to them in the foreseeable future or shall we close them for the time being?

[tapanchudasama]

@PyvesB I'll try to wrap this up by end of this week

[PyvesB]

Given that there hasn't been any activity for a while, I'm going to close this pull request. Feel free to ping us or reopen it if you get a chance to look into it again. 😉

If others think that repository level projects would be an option and want to pick this up, you can simply fetch the commits by running git fetch origin pull/7463/head:pr-7463. Change the implementation to target repository level projects and open a new pull request with a co-authored trailer included in the commit message to give attribution to the original author.