-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract examples from new-style services #1582
Extract examples from new-style services #1582
Conversation
Instead of centralizing examples, specify them from within a service.
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made a couple of suggestions, but even without them this is looking strong
|
||
const allBadgeExamples = require('./all-badge-examples'); | ||
|
||
describe('The badge examples', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think its worth having another test here which just iterates over all examples (rather than just the appveyor ones) and runs prepareExample()
on each one. It doesn't have to assert anything other than an error wasn't thrown. That way we'll get a failure on the server tests if an invalid example (without a previewUri
) is added in a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's a good idea, though I think this is already happening when this file is require
d, on account of the call to loadExamples()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one step ahead of me :)
services/appveyor/appveyor.js
Outdated
name: 'Branch', | ||
uri: '/appveyor/ci/gruntjs/grunt/master', | ||
title: 'branch', | ||
previewUri: '/appveyor/ci/gruntjs/grunt/master', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minor duplication, but given we define a base URI pattern in the service class I wonder if there is a way we could define the examples here as gruntjs/grunt/master
or { 'repo': 'gruntjs/grunt', 'branch': 'master' }
without needing to repeat /appveyor/ci
in each example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that makes a lot of sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an update. Could you take a look?
It's cool that you brought up { 'repo': 'gruntjs/grunt', 'branch': 'master' }
. I've started some experimentation with path-to-regexp which has a compile function that will help with this. I didn't quite get it working but my progress was promising!
I'll will have a proper look at this one tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go 👍
This cleans up the work from #1582, clarifying concerns, removing a bit of duplication, and renaming for clarity.
Nice! Thanks for continuing to work on this 😃 |
Instead of centralizing examples, specify them from within a service.