[HackerNews] Show User Karma #7411
Conversation
|
Actually its my second. 😄 First was this. But this is a new account so it might seem new. |
| ] | ||
|
|
||
| static defaultBadgeData = { | ||
| label: 'HackerNews User Karma', |
There was a problem hiding this comment.
This is a bit of a mouthful for a label, and I'd argue, going against the grain of the branding/promotional clause in our badge specification. There are a few cases where our default badge label has the service name included because we felt it was necessary to qualify/describe the value in the message.
However, I'm not sure that's the case here. Consider in reference our other social-style badges, including the Reddit Karma badge which is combined karma (I'm not sure what "combined" means in that context, but the main point is that the label doesn't include "Reddit").
https://shields.io/category/social
I think we should change this to:
| label: 'HackerNews User Karma', | |
| label: 'User Karma', |
or
| label: 'HackerNews User Karma', | |
| label: 'Karma', |
| transform({ json }) { | ||
| if (json == null) { | ||
| throw new NotFound({ prettyMessage: 'user not found' }) | ||
| } | ||
|
|
||
| const { karma, id } = json | ||
| return { karma, id } | ||
| } | ||
|
|
||
| async handle({ userid }) { | ||
| const json = await this.fetch({ userid }) | ||
| const { karma, id } = this.transform({ json }) | ||
| return this.constructor.render({ | ||
| karma, | ||
| userid: id, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Is there a difference in the id value in the response vs the userid value we already have via the route parameter?
If not then perhaps consider doing the null check inline with the same early throw/bail, and then calling the render function (with or without the deconstructing).
It's great to see the application of the common transform pattern, especially when we have complex logic that's much more easily tested directly via a unit test. However, in this case the canonical not found scenario can be easily exercised already and we aren't really doing anything else but deconstructing and then building a new object.
Just food for thought, don't feel obligated to make any changes
There was a problem hiding this comment.
Yep, this makes sense. transform does not do much work apart from throwing Error. If it was doing some object manipulation, then it would make sense to use it.
I think we can remove it.
| static examples = [ | ||
| { | ||
| title: 'HackerNews User Karma', | ||
| namedParams: { id: 'drumstick' }, |
There was a problem hiding this comment.
This example give me 'user not found'. Lets replace it with one that does
| namedParams: { id: 'drumstick' }, | |
| namedParams: { id: 'pg' }, |
| given(22222222222222222222222222).expect('22Y') | ||
| given(-999).expect('-999') |
There was a problem hiding this comment.
lets add a test for zero in here too
| given(22222222222222222222222222).expect('22Y') | |
| given(-999).expect('-999') | |
| given(22222222222222222222222222).expect('22Y') | |
| given(0).expect('0') | |
| given(-999).expect('-999') |
|
nice - thanks for your contribution |
|
Question here: I assumed since this has been merged, that I'd be able to use it on shields.io. But when I search for "hn" or "hacker news", I get no results. Am I missing something? |
|
It does exist. Example badge https://img.shields.io/hackernews/user-karma/pg?style=social call If you search "hackernews", or just "hacker" it shows up. That is.. not amazing, but if you wanted to submit a PR to add a few extra keywords, feel free. Here's an example of a service that defines some extra keywords for the examples: shields/services/piwheels/piwheels-version.service.js Lines 23 to 46 in fbb2ff3 |
No description provided.