Move build badge examples into services/ #2234
Conversation
all-badge-examples is a common cause of merge conflicts. It’s difficult to adjust the badge categorization in that file – or to understand the diff – because it requires moving a block from one point to another. It’s much easier to edit a badge’s category in one place. This starts the process of breaking up what’s left of that file, following up on the work from #1931.
paulmelnikow
changed the title
Generated by 🚫 dangerJS |
paulmelnikow
changed the title
There was a problem hiding this comment.
I've had a look over this on staging by eye. I've left one inline comment which is an easy fix. I also found the dockbit and bitrise examples aren't rendering, but I'm not quite sure why. It seems like it might be something to do with the colon character being URLencoded as %3a, but its only in the template URL. It can't be a coincidence though.
Annoyingly, now I see this I remember we did notice this in an earlier PR review #1740 (review) but I think it got lost in that review somewhere and we didn't do anything about it :(
Apart from that, everything else seems to be where it should be..
services/lgtm/lgtm-grade.service.js
Outdated
| return [ | ||
| { | ||
| title: 'LGTM Grade', | ||
| previewUrl: '/lgtm/grade/java/g/apache/cloudstack', |
There was a problem hiding this comment.
The LGTM Grade example isn't rendering - I suspect this is because starting this with a / is giving us an extra / char in the URL: /grade//lgtm/ (it would be nice to be more lenient about this)
There was a problem hiding this comment.
Heh, it's not just the slash; I forgot to strip off /lgtm/grade/ so that's showing up twice in the URL. 😝 Fixed.
Buh. Yea. The example URLs are wrong too. Part of it is the encoding, and the other part is putting the Offline we discussed running the test from #1800 (comment) to check these programmatically too. I'll work on that now. |
paulmelnikow
changed the title
Aah yeah - that'll be it. We probably need to be able to pass a |
|
I don't think moving them back to all-badge-examples will solve the problem. As far as I can tell, dockbit was broken before the move. I'm working on a little refactor of We'll be able to change the schema more easily once we empty out all-badge-examples – looking forward to that! |
I went down a rabbit hole while trying to untangle the bug in the dockbit and bitrise examples #2234 (review). The URL generation code is spaghetti-like, with functions, many of which I wrote, with opaque names, doing similar but not identical things, and making slightly incompatible assumptions about the way query strings are handled. I got a bit lost and need to take a step back. Meanwhile, this is a smalle piece of work I did that’s worth keeping. It doesn’t scratch the surface of the tangle, but it does remove a bit of duplication. It also makes a minor stylistic ES6 change in the handling of default arguments.
|
Ah, after running the automated test I see dockbit and bitrise got more broken by the move. I pushed a fix for the new issue with these two badges, which is the misplaced |
|
I used your script to compare the previewUrls in master and in the branch. This is the diff:
I repeated the test for This should be good to go! I'm going to disable the service tests because they're failing for unrelated reasons (#2239). |
paulmelnikow
changed the title
Continuing the work from #2234, this creates additional, empty LegacyServices to hold the badge examples for conda and cocoapods. It's an approach we could take to finish emptying out all-badge-examples while the refactoring continues. On the website badge, even the first URL path component is variable. I didn't think it could be moved, but it can!
I went down a rabbit hole while trying to untangle the bug in the dockbit and bitrise examples #2234 (review). The URL generation code is spaghetti-like, with functions, many of which I wrote, with opaque names, doing similar but not identical things, and making slightly incompatible assumptions about the way query strings are handled. I got a bit lost and need to take a step back. Meanwhile, this is a small piece of work I did that’s worth keeping. It doesn’t scratch the surface of the tangle, but it does remove a bit of duplication. It also makes a minor stylistic ES6 change in the handling of default arguments. Ref: #2027
all-badge-examples is a common cause of merge conflicts. It’s difficult to adjust the badge categorization in that file – or to understand the diff – because it requires moving a block from one point to another. It’s much easier to edit a badge’s category in one place.
This starts the process of breaking up what’s left of that file, following up on the work from #1931. New-style services can only be in one category, which means legacy service examples have to be split along category lines. I split out separate legacy service classes where I could do so easily, leaving behind the ones which require more work, for one reason or another.
I did this manually, which wasn't as bad as I thought. Doing it one category at a time made it easier. Also someone must have done some work to harmonize the keys. 😁(That wasn't I, was it?)
Anyway, these were done manually, so I'll check them on the deploy app. If someone else wants to do that too, an extra set of eyes wouldn't hurt!