Fix bug in website badge, allow specifying website name (left part of badge) #949

SuzanneSoy · 2017-04-19T14:01:48Z

I double-checked the regexp, and realised that there was still a bug when the up-down and colour options were specified. Sorry about that, hopefully this regexp fixes these issues (I tested it more thoroughly at https://regex101.com/r/xTAHin/1).

I also added support for specifying the left part of the badge, so that one can easily make badges that look like docs|online, downloads|online, official site|up and so on. This change is backwards-compatible, as it can only occur when an odd number of options is present (either SiteName, or up-down-SiteName, or up-down-green-red-SiteName, in addition to the previously supported formats “empty”, up-down and up-down-green-red which all had an even number of options).

Could you have a look at this PR?

paulmelnikow · 2017-04-19T19:01:54Z

Hi! Thanks for this.

Could you include a couple examples of badges which should work, which fail with the old regex?

Did #720 break something that used to work?

SuzanneSoy · 2017-04-19T21:51:38Z

@paulmelnikow Sure!

In /website-up-down-green-red/http/shields.io/foo/bar/baz.svg, the "old" regex (i.e. as it is currently, after applying #720) captured red/http/shields.io/foo as the colour, instead of stopping at the slash. It then captured bar as the protocol, and baz as the domain name.

The problem was that I hadn't excluded slashes in the "up/down" and "colour" capture groups, so the last one could greedily gobble past the following slash.

I don't think #720 broke anything that used to work, as before, if would simply fail to parse correctly anything with subdirectories after the domain name. #720 fixed that when no options (up/down text + colour) were given, but I had overlooked the fact that when options are given, they must not contain slashes.

This PR (#949) fixes that problem, allows specifying the text on the left part of the badge instead of the rather generic "website", so that one can have a docs|online badge, allows for double-slashes which escape to single slashes (in the same way as double dashes escape as single dashes) for the three texts (up text, down text and website text), as well as for the colours, for consistency (although it doesn't make much sense to have a slash in a colour name, it does no harm to allow it).

paulmelnikow · 2017-04-20T13:35:43Z

index.html

-    <td><img src='https://img.shields.io/website-up-down-green-red/http/shields.io.svg?maxAge=2592000' alt=''/></td>
-    <td><code>https://img.shields.io/website-up-down-green-red/http/shields.io.svg</code></td>
+    <td><img src='https://img.shields.io/website-up-down-green-red-my--website/http/shields.io.svg?maxAge=2592000' alt=''/></td>
+    <td><code>https://img.shields.io/website-up-down-green-red-my--website/http/shields.io.svg</code></td>


Could you move these changes to try.html? Sorry, but this file is autogenerated from that one.

paulmelnikow · 2017-04-20T13:40:16Z

server.js

+  return escapeFormat(t)
+    // Double slash
+    .replace(/\/\//g, '//');
+}


Conflicts with #952? @Daniel15

#952 does not seem to move the definition of escapeFormat, so I suppose this should be okay, right?

paulmelnikow · 2017-04-20T13:43:19Z

server.js

+  var offlineMessage = escapeFormatSlashes(match[4] != null ? match[4] : "offline");
+  var onlineColor = escapeFormatSlashes(match[7] != null ? match[7] : "brightgreen");
+  var offlineColor = escapeFormatSlashes(match[9] != null ? match[9] : "red");
+  var websiteName = escapeFormatSlashes(match[12] != null ? match[11] : "website");


Nit: Calling this label would make the intent clearer.

paulmelnikow · 2017-04-20T13:49:22Z

I did some quick testing:

Works:

Expected to work, but doesn't:

SuzanneSoy · 2017-04-20T14:09:16Z

@paulmelnikow I applied the changes you suggested on the diff (and used the term "label" in the documentation too, as it is indeed clearer).

I'm trying to install shields.io and its dependencies to figure out why some of the URLs still don't work.

SuzanneSoy · 2017-04-20T14:38:06Z

@paulmelnikow I'm trying to install shields inside a vanilla Ubuntu 14.04 VirtualBox machine, using these commands (from the INSTALL.md file):

sudo apt-get update && sudo apt-get install nodejs-legacy npm git
git clone https://github.com/badges/shields.git
cd shields/
sudo npm install
sudo node server

However, I get the following error (when trying to run a checkout of the master branch):

/home/virtualbox/shields/server.js:299
  camp,
      ^
SyntaxError: Unexpected token ,
    at Module._compile (module.js:439:25)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:902:3

Did I miss some obvious step somewhere?

paulmelnikow · 2017-04-20T14:55:14Z

Ah, you need Node 6 or later: nvm install 6 && nvm alias default 6

Feel free to PR those instructions, if you'd like.

paulmelnikow · 2017-04-20T14:56:38Z

By the way, what's your dev machine? You probably can run it directly.

SuzanneSoy · 2017-04-20T15:12:00Z

@paulmelnikow Thanks, I opened a PR for INSTALL.md here: #955.

I'm running NixOS, which was a somewhat unfortunate decision for a development machine. It has a completely write-protected /usr tree, even for sudo, and stock executables cannot be run directly, they must be patched. It also isolates applications in a rather strict way, to force all dependencies to be explicit. This allows NixOS to have a "purely functional" package manager, guaranteeing that a given configuration will always yield the same installed files, and allowing multiple versions of libraries to coexist. It sounds great on paper, but in practice it is a bit of a pain as installing any non-packaged tool which ships binaries requires patching them, and specifying their list of dependencies. So usually, I don't even bother, and pop up a VM :) .

…was improperly parsed). Allow specifying website name (left part of the badge).

SuzanneSoy · 2017-04-20T15:51:17Z

@paulmelnikow I tested the three problematic URLs that you mentioned, there were a few small issues (an extra dash before the label, and double slashes were not properly un-escaped, and incorrect handling of the label-only case when it contains dashes), but aside from these issues they seem to work.

Are you sure you were on the right branch when testing?

I tried with these urls, each giving the the badge following it:

http://127.0.0.1/website-si--t--e//do//--cs/https/img.shields.io/nonexistant/foo-bar-green.svg.svg
si-t-e/do/-cs|offline (red)
http://127.0.0.1/website-si--t--e//do//--cs/https/img.shields.io/badge/foo-bar-green.svg.svg
si-t-e/do/-cs|online (green)
http://127.0.0.1/website-up-down-si--t--e//do//--cs/https/img.shields.io/nonexistant/foo-bar-green.svg.svg
si-t-e/do/-cs|down (red)
http://127.0.0.1/website-up-down-si--t--e//do//--cs/https/img.shields.io/badge/foo-bar-green.svg.svg
si-t-e/do/-cs|up (green)
http://127.0.0.1/website-up-down-blue-orange-si--t--e//do//--cs/https/img.shields.io/nonexistant/foo-bar-green.svg.svg
si-t-e/do/-cs|down (orange)
http://127.0.0.1/website-up-down-blue-orange-si--t--e//do//--cs/https/img.shields.io/badge/foo-bar-green.svg.svg
si-t-e/do/-cs|up (blue)
http://127.0.0.1/website---u--//p---d--own//-blue-orange-si--t--e//do//--cs/https/img.shields.io/badge/foo-bar-green.svg.svg
si-t-e/do/-cs|-u-/p- (blue)
http://127.0.0.1/website---u--//p---d--own//---blue-orange-si--t--e//do//--cs/https/img.shields.io/nonexistant/foo-bar-green.svg.svg
si-t-e/do/-cs|d-own/- (orange)

In principle, it is not possible to make the "down" or "label" parts start with a double dash, as the meaning is ambiguous and the dash ends up as the last character of the preceding group. However this is a rather unlikely use case, and a zero-width no-break-space can be used to disambiguate, without affecting the graphical output.

http://127.0.0.1/website---u--//p-%EF%BB%BF--d--own//---blue-orange-%EF%BB%BF--si--t--e//do//--cs/https/img.shields.io/nonexistant/foo-bar-green.svg.svg
-si-t-e/do/-cs|-d-own/- (orange)

Of course, these are contrived examples, and hopefully most people should be plenty satisfied by the following :)

http://127.0.0.1/website-online-unavailable-blue-red-docs/https/en.wikipedia.org/wiki/Documentation.svg

paulmelnikow · 2017-04-20T18:19:44Z

Thanks for the thorough testing! All the examples are working now. Seems entirely possible I was on the wrong branch. Whoops!

After #937 is merged, would be great to circle back and add some good tests of this badge, which seems particularly tricky. Possibly a test of the helper functions would help to clarify why they exist.

Daniel15 · 2017-04-20T18:25:13Z

@jsmaniac - For what it's worth, you should also be able to use Docker. There's a Dockerfile in this repo.

- Build index.html at deploy time - Update corresponding documentation references - Since index.html is untracked, git add needs -f - Clarify gh-pages generated commit message - Improve Makefile dependencies related to website generation As discussed in #936, tracking the index.html causes makes PRs longer / noisier and causes extra merge conflicts. More importantly, it causes contributors to inadvertently edit the wrong file, which causes extra work (#949) or contributions to be lost (#898). Since there's no need for index.html in development (everything uses try.html) a logical solution is to generate and commit the index.html at deploy time. Recording compiled or generated files in a deploy commit is a reasonable practice for git-based deploys (Heroku, gh-pages, and others). The old version of this was slightly "unsafe" for my taste, in that it depended on the local copy of gh-pages (if it existed) and master. The new version just replaces gh-pages with master + the new commit. Closes #936. Fixes #954 (the PR).

paulmelnikow · 2017-04-26T15:49:24Z

@jsmaniac I realized ?label=docs should accomplish the same thing as the label feature in the URI. Would you like to try that out? Maybe this code can be simplified a bit. If we move fast, it seems fine to remove that feature from the URI, even though it's a breaking change.

SuzanneSoy · 2017-04-26T17:06:26Z

@paulmelnikow Indeed, I didn't know there was a ?label=… option, is that a recent addition?

I just pushed an update which removes the label-handling code for website to https://github.com/jsmaniac/shields/tree/website-badge2 ; I'll run a few tests, and make a PR after checking that everything still works.

paulmelnikow · 2017-04-26T17:26:04Z

I didn't either! The logic is in getBadgeData, and it's documented at the bottom of https://shields.io/. Support would be patchy depending on the implementation of the badge. In many cases the badge handler overrides some of the parameters, though in others they all work as intended.

paulmelnikow added bug Bugs in badges and the frontend service-badge New or updated service badge labels Apr 19, 2017

paulmelnikow reviewed Apr 20, 2017

View reviewed changes

SuzanneSoy added a commit to SuzanneSoy/shields that referenced this pull request Apr 20, 2017

Cleaned up PR badges#949

9328164

paulmelnikow mentioned this pull request Apr 20, 2017

Do not commit the index.html file. #936

Closed

5 tasks

paulmelnikow mentioned this pull request Apr 20, 2017

Build index.html at deploy time #954

Closed

SuzanneSoy force-pushed the website-badge branch from 38c47f5 to aff1488 Compare April 20, 2017 15:27

Fix bug in website badge (specifying colours & test + subdirectories …

8087843

…was improperly parsed). Allow specifying website name (left part of the badge).

SuzanneSoy force-pushed the website-badge branch from aff1488 to 8087843 Compare April 20, 2017 15:51

paulmelnikow merged commit 571f46f into badges:master Apr 20, 2017

paulmelnikow added the pending-deploy label Apr 20, 2017

paulmelnikow added this to the Next Deploy milestone Apr 26, 2017

SuzanneSoy mentioned this pull request Apr 26, 2017

Removed ability to specify the badge's label for the “website” service #965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in website badge, allow specifying website name (left part of badge) #949

Fix bug in website badge, allow specifying website name (left part of badge) #949

SuzanneSoy commented Apr 19, 2017 •

edited

Loading

paulmelnikow commented Apr 19, 2017

SuzanneSoy commented Apr 19, 2017 •

edited

Loading

paulmelnikow Apr 20, 2017

paulmelnikow Apr 20, 2017

SuzanneSoy Apr 20, 2017

paulmelnikow Apr 20, 2017

paulmelnikow commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

paulmelnikow commented Apr 20, 2017

paulmelnikow commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

paulmelnikow commented Apr 20, 2017

Daniel15 commented Apr 20, 2017

paulmelnikow commented Apr 26, 2017

SuzanneSoy commented Apr 26, 2017

paulmelnikow commented Apr 26, 2017

Fix bug in website badge, allow specifying website name (left part of badge) #949

Fix bug in website badge, allow specifying website name (left part of badge) #949

Conversation

SuzanneSoy commented Apr 19, 2017 • edited Loading

paulmelnikow commented Apr 19, 2017

SuzanneSoy commented Apr 19, 2017 • edited Loading

paulmelnikow Apr 20, 2017

Choose a reason for hiding this comment

paulmelnikow Apr 20, 2017

Choose a reason for hiding this comment

SuzanneSoy Apr 20, 2017

Choose a reason for hiding this comment

paulmelnikow Apr 20, 2017

Choose a reason for hiding this comment

paulmelnikow commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

paulmelnikow commented Apr 20, 2017

paulmelnikow commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

SuzanneSoy commented Apr 20, 2017

paulmelnikow commented Apr 20, 2017

Daniel15 commented Apr 20, 2017

paulmelnikow commented Apr 26, 2017

SuzanneSoy commented Apr 26, 2017

paulmelnikow commented Apr 26, 2017

SuzanneSoy commented Apr 19, 2017 •

edited

Loading

SuzanneSoy commented Apr 19, 2017 •

edited

Loading