add Opera Add-ons support #990

perceptron8 · 2017-05-10T18:14:29Z

resolves #827

http://eslint.org/docs/rules/no-case-declarations

perceptron8 · 2017-05-12T10:05:23Z

Don't merge yet, please. I'm going to switch from html-soup to cheerio. I didn't know such lib exists.

Daniel15 · 2017-05-16T19:02:42Z

Please let us know when this is ready for review :)

perceptron8 · 2017-05-16T20:10:38Z

It is already! :)

perceptron8 · 2017-05-21T09:33:46Z

Now it's even more ready.

timbru31 · 2017-05-24T08:18:00Z

Nice, one step further to unified browser extensions 🎉

perceptron8 · 2017-06-03T13:24:44Z

@Daniel15 Please let me know when you are ready for review.

Daniel15 · 2017-06-05T05:43:06Z

Sorry for the delay, I've been pretty busy recently :( I'll try to view this soon, but someone else may get to it before I do.

Daniel15

Sorry for the delay!

Daniel15 · 2017-06-27T04:23:12Z

server.js

+      try {
+        const cheerio = require('cheerio');
+        const $ = cheerio.load(buffer);
+        const version = $('section.about > dl > dd').eq(2).text();


This seems very unmaintainable and will break if Opera ever change their HTML even slightly. Do Opera not have an API for retrieving this data?

Something as simple as Opera switching the order of the fields in the "about the extension" section (for example, displaying "size" above "version") will break this. If you really do need to parse the HTML, it might be worth making it smarter (eg. look for the <dt> containing "Version" and then look in the <dd> directly after that)

I'd suggest pulling this out into a separate utility module (eg. lib/parseOperaExtensionPage.js) rather than putting all the code in here.

Daniel15 · 2017-06-27T04:26:38Z

server.js

+        case 'rating-count':
+          badgeData.text[0] = data.label || 'rating-count';
+          badgeData.text[1] = metric(reviewCount) + ' total';
+          badgeData.colorscheme = floorCountColor(reviewCount, 5, 50, 500);


A lot of this code looks like it was copied and pasted from the Chrome web store implementation. Instead of copying and pasting it, could you pull it out into a reusable utility function?

Daniel15 · 2017-06-27T04:31:17Z

Please also add test cases for this. See https://github.com/badges/shields/tree/master/service-tests for instructions on how to write tests.

perceptron8 · 2017-06-27T06:52:44Z

@Daniel15 Thanks you for your comments. Give me some time, please.

Daniel15 · 2017-06-27T06:53:19Z

No worries, feel free to take as long as you like! No pressure 😄

perceptron8 · 2017-07-02T09:26:34Z

This seems very unmaintainable and will break if Opera ever change their HTML even slightly. Do Opera not have an API for retrieving this data?

There is no public API for Opera Add-ons, neither for Chrome Web Store.

Something as simple as Opera switching the order of the fields in the "about the extension" section (for example, displaying "size" above "version") will break this. If you really do need to parse the HTML, it might be worth making it smarter.

No need to worry. Tests will catch that. It's much more likely that store's layout changes completely than "size" goes above "version".

Please also add test cases for this.

Done.

I'd suggest pulling this out into a separate utility module (eg. lib/parseOperaExtensionPage.js) rather than putting all the code in here.

Parsing is just 4 lines of code. There's much more ceremony with badge construction: that's what should be improved in the first place. I'll be happy to take care of opera and chrome modules as soon as server.js becomes modular (in the sense of #948 and #963).

I did, however, some refactoring in color- and text-formatters. It was painful to replace all versionFormatter() calls but someone had to do this.

Daniel15 · 2017-07-10T01:10:52Z

lib/text-formatters.js

-  while (stars.length < rating) { stars += '★'; }
-  while (stars.length < 5) { stars += '☆'; }
-  return stars;
+  assert(rating >= 0);


What happens at runtime if the assertion fails?

An AssertionError is thrown with a default message. No one should invoke this method with negative number. An exception signals function caller a mistake. It's fail-fast vs return garbage in / gargage out.

Also note that rounding changed from Math.ceil (implicit) to Math.round (explicit), so starRating(1.01) gives ★☆☆☆☆ instead of ★★☆☆☆.

Daniel15 · 2017-07-10T01:11:11Z

package.json

@@ -32,6 +33,7 @@
    "pdfkit": "~0.8.0",
    "redis": "~2.6.2",
    "request": "~2.81.0",
+    "request-promise": "~4.2.1",


Are you using this?

https://github.com/perceptron8/shields/blob/e3bc0573db9410f7fe3f8b05d6f218438ac2eb7e/server.js#L29
https://github.com/perceptron8/shields/blob/e3bc0573db9410f7fe3f8b05d6f218438ac2eb7e/server.js#L5664
https://github.com/perceptron8/shields/blob/e3bc0573db9410f7fe3f8b05d6f218438ac2eb7e/server.js#L5706

paulmelnikow · 2017-09-22T04:58:49Z

@espadrine has argued against parsing on the server because it's resource intensive. We've said no to badges like these before, and asked that contributors approach the vendor and ask them to create an API.

This PR has a bunch of unrelated changes / refactoring. Could those be cherry-picked into a new PR?

perceptron8 · 2017-09-22T06:51:30Z

@paulmelnikow You also said yes to badges like this before, so I don't understand why this is so problematic now and it wasn't in case of Firefox and Chrome. After nearly 11 months I have no expectations really, but - at least be fair.

As for the question, I'll be happy to see that versionFormatter() is gone forever. Be my guest. Pick whatever you want.

paulmelnikow · 2017-09-22T14:52:24Z

As far as I know I haven't merged a badge that parses web pages. I did not realize the Chrome badge used a library that uses cheerio.

I don't have a problem with it from a maintainability perspective, as integration tests can catch the problem, with the caveat that our deploy cycles can be a bit long. If there's not a performance problem I am okay with it.

@Daniel15 it sounds like you are okay with it too. @espadrine what do you think?

paulmelnikow · 2017-12-07T21:04:42Z

I know you're frustrated about this. I apologize. There's not much else I can say, other than this (from #358 (comment)):

We need more capacity, or confidence we can buy more capacity, before we add more scrapers.

Our OpenCollective is now open, which means we can start to collect $10 donations toward this fund. If you wanted an answer to "what can you do to make this happen," it's to spread that message, far and wide.

perceptron8 added 2 commits May 10, 2017 20:10

add Opera Add-ons support

1cc1bbd

resolves #827

fix eslint problems

746c874

http://eslint.org/docs/rules/no-case-declarations

perceptron8 added 2 commits May 12, 2017 12:55

switch from html-soup to cheerio

d880d03

get rid of console.log

48c3791

Daniel15 added the service-badge New or updated service badge label May 16, 2017

badge colorscheme based on no-rounded rating values

628d7cc

Daniel15 reviewed Jun 27, 2017

View reviewed changes

perceptron8 added 3 commits July 1, 2017 21:21

add rating-related utility functions, split versionColor, refactor

602663e

remove chrome web store price

bc0861e

tests for chrome and opera

e3bc057

Daniel15 reviewed Jul 10, 2017

View reviewed changes

PyvesB mentioned this pull request Nov 14, 2017

Badge request: Google Play Store #358

Open

espadrine force-pushed the master branch from 203d6d5 to 7e668c7 Compare December 3, 2017 18:06

paulmelnikow closed this Dec 7, 2017

calebcartwright mentioned this pull request Jan 9, 2021

Microsoft Edge & Opera #6041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Opera Add-ons support #990

add Opera Add-ons support #990

perceptron8 commented May 10, 2017

perceptron8 commented May 12, 2017

Daniel15 commented May 16, 2017

perceptron8 commented May 16, 2017

perceptron8 commented May 21, 2017

timbru31 commented May 24, 2017

perceptron8 commented Jun 3, 2017

Daniel15 commented Jun 5, 2017

Daniel15 left a comment

Daniel15 Jun 27, 2017

Daniel15 Jun 27, 2017

Daniel15 Jun 27, 2017

Daniel15 commented Jun 27, 2017

perceptron8 commented Jun 27, 2017

Daniel15 commented Jun 27, 2017

perceptron8 commented Jul 2, 2017

Daniel15 Jul 10, 2017

perceptron8 Jul 10, 2017 •

edited

Loading

perceptron8 Jul 10, 2017

Daniel15 Jul 10, 2017

perceptron8 Jul 10, 2017

paulmelnikow commented Sep 22, 2017

perceptron8 commented Sep 22, 2017

paulmelnikow commented Sep 22, 2017

paulmelnikow commented Dec 7, 2017

add Opera Add-ons support #990

add Opera Add-ons support #990

Conversation

perceptron8 commented May 10, 2017

perceptron8 commented May 12, 2017

Daniel15 commented May 16, 2017

perceptron8 commented May 16, 2017

perceptron8 commented May 21, 2017

timbru31 commented May 24, 2017

perceptron8 commented Jun 3, 2017

Daniel15 commented Jun 5, 2017

Daniel15 left a comment

Choose a reason for hiding this comment

Daniel15 Jun 27, 2017

Choose a reason for hiding this comment

Daniel15 Jun 27, 2017

Choose a reason for hiding this comment

Daniel15 Jun 27, 2017

Choose a reason for hiding this comment

Daniel15 commented Jun 27, 2017

perceptron8 commented Jun 27, 2017

Daniel15 commented Jun 27, 2017

perceptron8 commented Jul 2, 2017

Daniel15 Jul 10, 2017

Choose a reason for hiding this comment

perceptron8 Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

perceptron8 Jul 10, 2017

Choose a reason for hiding this comment

Daniel15 Jul 10, 2017

Choose a reason for hiding this comment

perceptron8 Jul 10, 2017

Choose a reason for hiding this comment

paulmelnikow commented Sep 22, 2017

perceptron8 commented Sep 22, 2017

paulmelnikow commented Sep 22, 2017

paulmelnikow commented Dec 7, 2017

perceptron8 Jul 10, 2017 •

edited

Loading