Add support for [Localazy] badge #5701

honzabilek4 · 2020-10-13T09:36:55Z

shields-ci · 2020-10-13T09:40:00Z

	Messages
📖	✨ Thanks for your contribution to Shields, @honzabilek4!

Generated by 🚫 dangerJS against 0cebcc1

calebcartwright · 2020-10-15T02:31:06Z

Thanks for the PR @honzabilek4! I've pulled this up a couple times now planning to review, but the diff is on the larger side which complicates things a bit.

I think this is largely driven by the number of distinct service class definitions and their respective tester files. Could you provide some additional context here on the badge(s) being added? I feel like that might make things easier for us to review and see if we couldn't potentially simplify the code while still achieving the same results.

I've deployed a review app for this PR that can be used to include rendered badges from your branch if that helps

For example, I feel like we could probably have a single class for the language progress badges, and just use a query parameter on the badge route that has three variants which control how the label is rendered (english, locale, locale name)

So instead of
https://shields-staging-pr-5701.herokuapp.com/localazy/lang-progress-en/floating-apps/pt_BR

https://shields-staging-pr-5701.herokuapp.com/localazy/lang-progress-loc/floating-apps/pt_BR

We could just have something like the below for the three variants, all with a single class

https://shields-staging-pr-5701.herokuapp.com/localazy/lang-progress/floating-apps/pt_BR?locale_display=loc

https://shields-staging-pr-5701.herokuapp.com/localazy/lang-progress/floating-apps/pt_BR?locale_display=name

honzabilek4 · 2020-10-22T17:27:26Z

Hi @calebcartwright,
Great idea! I originally wanted to create as few services as possible only to find out later that some combinations weren't possible. That's why I split it into each service per badge type, but your suggestion makes perfect sense. I'll try to reduce it a bit.

honzabilek4 · 2020-10-23T13:40:36Z

@calebcartwright I've updated the PR, please check it out 🙂

PyvesB

Thanks for your contribution to Shields @honzabilek4! Overall this looks quite good, I've left a few comments and thoughts inline. 😉

PyvesB · 2020-10-24T09:53:59Z

services/localazy/localazy-all-progress.service.js

+  ]
+
+  async handle({ projectId }) {
+    const { message, label, color } = await this.fetch({


Does this mean the Localazy API returns the colour itself? This seems a little odd to me, Shields.io generally decides it own colours through a set of conventions (e.g. colour gradients for percentages), I'm not sure what to think of an API imposing its own.

What do you think @calebcartwright ?

Agreed. If there's a need to use a custom scale that'd probably be okay, but the colors used for those tiers should follow our conventions to be consistent with our offering.

We return colour to quickly generate the badges on our side, though in this PR, it's always possible to override it as well as the other parameters.

What is the convention here? The gradient would certainly make sense for single language progress, even though it's not that simple as some code coverage for example. The rest was supposed to be just an informational value, since 100% on the project doesn't mean that the localization process is necessarily finished.

PyvesB · 2020-10-24T09:57:23Z

services/localazy/localazy-all.tester.js

+
+const t = (module.exports = require('../tester').createServiceTester())
+
+t.create('Localazy translation progress with langs')


Is there any specific reason why most of the tests used mocked responses? Apart from edge cases which are hard to encounter in real life, we generally prefer to hit the real service directly. This gives us much more confidence that the end-to-end flow is working correctly and allows us to be warned sooner rather than later when things unexpectedly change in upstream APIs.

Okay, I can remove the mocks, it just depends on what do you want to test here.

PyvesB · 2020-10-24T09:58:15Z

services/localazy/localazy-base.service.js

+  color: Joi.string(),
+})
+
+module.exports = class LocalazyBase extends BaseJsonService {


Nice extraction of common functionality. 👍🏻

PyvesB · 2020-10-24T10:03:49Z

services/localazy/localazy-langs-simple.service.js

+
+const LocalazyBase = require('./localazy-base.service')
+
+module.exports = class LocalazyLangsSimple extends LocalazyBase {


I'm personally not a fan of this badge variant. It's almost the same as the LocalazyLangs one, but less clear. The word "translated" is not quantifiable when used in isolation, which makes the badge somewhat confusing IMHO.

Maybe merge LocalazyLangsSimple and LocalazyLangs and use the noun form instead?

Yeah, makes sense. 🙂

PyvesB · 2020-10-24T10:07:43Z

services/localazy/localazy-lang-progress.service.js

+      },
+      staticPreview: this.render({
+        message: '100%',
+        label: 'Português (BR)',


I like the concept of displaying the language name in the language itself, that's a nice user experience. 👍🏻

calebcartwright · 2020-10-24T21:35:28Z

services/localazy/localazy-base.service.js

+  static get documentation() {
+    return `For more badges and detailed documentation of the params visit <a href="https://github.com/localazy/shields">localazy/shields</a> repository.`
+  }
+
+  static get keywords() {
+    return ['l10n', 'i18n', 'localization', 'internationalization']
+  }


Could you convert these to static fields like the rest (category, route, etc.) please?

calebcartwright · 2020-10-24T21:40:16Z

services/localazy/localazy-base.service.js

+const schema = Joi.object({
+  label: Joi.string(),
+  message: Joi.string(),
+  color: Joi.string(),
+})


I'm a little surprised by this schema, as it has a very Shields.io feel to it 😄 Are we utilizing an API here that's been designed specifically with our schema in mind or does the native Localazy API just happen to have contracts structured this way?

It's following the shields.io structure, so we can easily proxy the calls to your api and generate the badges on-the-fly.
https://connect.localazy.com/status/floating-apps?title=lang-loc-name&content=fr_CA

Same API is then used by the services here to fetch the raw data.

It's following the shields.io structure, so we can easily proxy the calls to your api and generate the badges on-the-fly.

I'm confused by this. Could you elaborate on what you mean by "your api"?

If I was a user of the Localazy platform, and I wanted to integrate with it programmatically how would I go about doing that, what Localazy API(s) would I use?

What's currently proposed here is a bit concerning because it looks like the core aspects of the badge rendering are being controlled by the connect.localazy.com endpoint being used which I oppose.

Our core mission is providing 'concise, consistent, and legible' badges, and we do that by retrieving the raw data from the upstream system of record and using that data to produce the badges according to our specification and conventions. Ceding control of core default attributes like the badge label, message, and colors to some external endpoint is not going to be acceptable IMO.

We've put the api together quickly for this purpose, so users can start using any badges before this gets merged.
https://github.com/localazy/shields

That's why the api copies the structure. We return these values directly for convenience and faster integration with shields.io.
We don't impose how the badge should look like, though we need to have some defaults. Users can always modify the overal look, title, colour etc. from within shields.io.

I can move the defaults directly to the repo if that's an issue.

We don't impose how the badge should look like, though we need to have some defaults.

We have a suggested path on how we think we can best proceed, though I want to first do some contextual level setting that I think will be helpful.

I tend to view our service's badge offerings in two high level buckets:

Our native/core badges that provide dyanmically updated badges that use raw data from various upstream tools/services/platforms, and do so in a way that's consistent

Our build-your-own badge (BYOB) features that come in three flavors:
A. static badge builder (https://img.shields.io/badge/foo-bar-blue)
B. dynamic xml/json/yaml badge builder. this is similar to the static badge builder, except that the values can be set dynamically using xpath/jsonpath to query an arbitrary document instead of fixed/hard-coded
C. Endpoint badges (https://shields.io/endpoint / Announcing the Endpoint badge (beta) #2838) I like to think of this as a more powerful/flexible extension of the dynamic badge offering

That first bucket is the core of what we do, and is a large part of why Shields exists in the first place (more historical background here). We ensure that regardless of which platform/service is in use that the badges are consistent, both in specification and in presentation of information such as metrics and percentages, and status text like the result of a build. In order to be able to provide that consistency across our native badge offerings, we have to control core attributes of those badges we're providing which includes things like the defaults and text, color, etc. This is how we're able to provide a standard build status badge regardless of whether a user is using CI Service Foo or CI Service Bar, and the same holds true for other types of badges like code overage or localization status/progress.

Yes, we do support some opt-in customization options that our users can utilize should they choose to override the defaults, but we still have to ensure that the defaults are consistent across our platform. If Shields were to incorporate a native/core badge but not have control over those core attributes then we'd no longer be able to enforce and ensure that consistency and could quickly and easily end up in a place where our native badges are directly violating our own badge specification

Ultimately the challenge with this PR in it's current state is that it's proposing adding native/core badges but doing so in the BYOB fashion which just isn't compatible. We can't add a native badge that has the label, message, and color set by some external endpoint that we have no insight into nor control over.

We'd love to collaborate with you all and be able to get Localazy incorporated in our native badge offering, though we'll need to pivot a bit to make that happen. The next step to achieving that would be switching the service class implementation to utilize a Localazy API endpoint that directly provides the raw localization progress data. I'm unclear on whether such an endpoint exists yet, but I believe @PyvesB had some suggestions for a contract if not. I also believe that such an endpoint is beneficial above and beyond badges to allow Localazy users to have an API option to interact with the platform indepenent of a badge-specific focus.

If such an API doesn't yet exist then we can also work on helping with the implementation on the Shields side to integrate a native badge if you are willing to tackle the API implementation side.

I hope this helps, but please let us know if you have any questions!

Hello @calebcartwright,
Thank you for thorough explanation! I know it can be tedious to maintain an open source project like this :)
I think I'll start with exploring the capabilities of our own API and update you on the progress.
I don't have any other questions generally.

From the point of view of a user of the API, I can picture two endpoints:

one endpoint requiring a project ID, which would return a response similar to the following:

{ "schemaVersion": 1, "translationProgress": 0.96, "translations": 5 }

one endpoint requiring a project ID and a language code, which would return a response similar to the following:

{ "schemaVersion": 1, "translationProgress": 0.97, "languageName": "French", "nativeLanguageName": "Français" }

This would allow Shields.io to be able to control how it uses the data returned by Localazy and ensure consistency with other badges it offers, but it would also allow other consumers of the API to easily make use of it in different ways. 👍🏻

Thanks for your suggestions. We'll do an update of our API and I'll get back to the PR as soon as possible.

PyvesB · 2021-05-04T19:27:10Z

Given that it's been a while, I'm going to go ahead close this pull request. Feel free to reopen it if you get a chance to look into it again, we would still be very keen to see Localazy badges added to Shields.io at some point! As a reminder, our thoughts on a way forward were summarised in #5701 (comment) and #5701 (comment).

Thanks again for your interest in the Shields.io project!

Jan Bílek added 6 commits October 8, 2020 20:07

Add basic localazy badges

2befc89

Split badges into services

f9a7096

Add first tests

f7af2bf

Add missing tests for services

bede797

Add test with real API call

508f03b

Update test name

dc65327

calebcartwright changed the title ~~Add support for Localazy badge~~ Add support for [Localazy] badge Oct 13, 2020

calebcartwright added the service-badge New or updated service badge label Oct 13, 2020

shields-cd temporarily deployed to shields-staging-pr-5701 October 15, 2020 01:57 Inactive

Merge branch 'master' into master

f6ac14a

shields-cd temporarily deployed to shields-staging-pr-5701 October 23, 2020 09:20 Inactive

Join lang progress services into one

98bde7b

shields-cd temporarily deployed to shields-staging-pr-5701 October 23, 2020 13:39 Inactive

Fix linter errors

0cebcc1

shields-cd temporarily deployed to shields-staging-pr-5701 October 23, 2020 13:40 Inactive

PyvesB reviewed Oct 24, 2020

View reviewed changes

calebcartwright reviewed Oct 24, 2020

View reviewed changes

shields-cd temporarily deployed to shields-staging-pr-5701 October 27, 2020 18:15 Inactive

PyvesB mentioned this pull request Mar 23, 2021

New Badge: Upptime #6307

Closed

This was referenced Apr 8, 2021

Add [Stryker] service #6368

Closed

Document stance on APIs following the endpoint badge schema #6370

Closed

PyvesB closed this May 4, 2021

snyk-bot mentioned this pull request Sep 14, 2021

[Snyk] Upgrade simple-icons from 4.15.0 to 4.25.0 kamil1marczak/shields#3

Open

snyk-bot mentioned this pull request May 24, 2022

[Snyk] Upgrade simple-icons from 4.11.0 to 4.25.0 TriForMine/shields#10

Open

calebcartwright mentioned this pull request Jun 10, 2022

Add [Sonatype Lift] badge #8047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for [Localazy] badge #5701

Add support for [Localazy] badge #5701

honzabilek4 commented Oct 13, 2020

shields-ci commented Oct 13, 2020 •

edited

Loading

calebcartwright commented Oct 15, 2020

honzabilek4 commented Oct 22, 2020

honzabilek4 commented Oct 23, 2020

PyvesB left a comment

PyvesB Oct 24, 2020

calebcartwright Oct 24, 2020

honzabilek4 Oct 26, 2020

honzabilek4 Oct 26, 2020

PyvesB Oct 24, 2020

honzabilek4 Oct 26, 2020

PyvesB Oct 24, 2020

PyvesB Oct 24, 2020

honzabilek4 Oct 26, 2020

PyvesB Oct 24, 2020

calebcartwright Oct 24, 2020

honzabilek4 Oct 26, 2020

calebcartwright Oct 24, 2020

honzabilek4 Oct 26, 2020

calebcartwright Oct 27, 2020

honzabilek4 Oct 27, 2020 •

edited

Loading

calebcartwright Oct 27, 2020 •

edited

Loading

honzabilek4 Oct 28, 2020

PyvesB Oct 28, 2020

honzabilek4 Oct 29, 2020

PyvesB commented May 4, 2021 •

edited

Loading


		const t = (module.exports = require('../tester').createServiceTester())

		t.create('Localazy translation progress with langs')


		const LocalazyBase = require('./localazy-base.service')

		module.exports = class LocalazyLangsSimple extends LocalazyBase {

Add support for [Localazy] badge #5701

Add support for [Localazy] badge #5701

Conversation

honzabilek4 commented Oct 13, 2020

shields-ci commented Oct 13, 2020 • edited Loading

calebcartwright commented Oct 15, 2020

honzabilek4 commented Oct 22, 2020

honzabilek4 commented Oct 23, 2020

PyvesB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honzabilek4 Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

calebcartwright Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PyvesB commented May 4, 2021 • edited Loading

shields-ci commented Oct 13, 2020 •

edited

Loading

honzabilek4 Oct 27, 2020 •

edited

Loading

calebcartwright Oct 27, 2020 •

edited

Loading

PyvesB commented May 4, 2021 •

edited

Loading