Skip to content
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

Rewrite vso, rename to [AzureDevops], validate SVG [readthedocs] #2252

Merged
merged 11 commits into from
Nov 5, 2018
Merged

Rewrite vso, rename to [AzureDevops], validate SVG [readthedocs] #2252

merged 11 commits into from
Nov 5, 2018

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Nov 1, 2018

This PR does a couple things:

  1. Add validation to BaseSvgScrapingService and update readthedocs accordingly.
  2. Rewrite vso and add more tests. Rename it internally to azure-devops. URLs are still /vso for now. Should we make a way to let a service register multiple URL patterns?
  3. Handle shared code using a functional pattern instead of inheritance. This comes from a discussion [WIP] Rewrite [vso] badges #2031 (comment). I like the functional approach because it's more direct, nimble, and easy to reason about; plus it allows services to grow from a family of one to two more easily.

- Rename vso to azure-devops internally
    - URLs are still /vso for now
        - Should we make a way to let a service register multiple URL patterns?
- Add validation to BaseSvgScrapingService
    - Update readthedocs
- Handle shared code using a functional pattern instead of inheritance (ref: #2031 (comment))
@paulmelnikow paulmelnikow added service-badge New or updated service badge core Server, BaseService, GitHub auth, Shared helpers labels Nov 1, 2018
@shields-ci
Copy link

Warnings
⚠️

This PR modified service code for readthedocs but not its test code.
That's okay so long as it's refactoring existing code.

Messages
📖

✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS

@paulmelnikow paulmelnikow changed the title Rewrite vso, rename to [azure-devops], validate SVG [readthedocs] Rewrite vso, rename to [AzureDevops], validate SVG [readthedocs] Nov 1, 2018
@chris48s
Copy link
Member

chris48s commented Nov 2, 2018

Should we make a way to let a service register multiple URL patterns?

The way we've handled this on other badges is to stick the alternatives in a non-capturing group

e.g:

static get url() {
return {
base: `wordpress/${extensionType}`,
format: '(?:stars|r)/(.+)',
capture: ['slug'],
}
}

so you could use (?:vso|azure-devops) to 'rebrand' the URLs /azure-devops but still allow /vso for backwards compatibility.

Handle shared code using a functional pattern instead of inheritance.

Seems reasonable 👍 Its a useful pattern to add to the toolbelt. I suspect its probably going to be most useful for larger service families where there is commonality but also variance as opposed to a rigid parent/child structure. I wouldn't particulary want to mandate style here though. Lets leave composition vs inheritance up to the implementer.

@paulmelnikow
Copy link
Member Author

I suspect its probably going to be most useful for larger service families where there is commonality but also variance as opposed to a rigid parent/child structure.

Could be true. I could also see it being valuable for single-service families which grow larger over time.

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2252 November 4, 2018 23:33 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2252 November 4, 2018 23:34 Inactive
@paulmelnikow
Copy link
Member Author

Thanks for reviewing!

@paulmelnikow paulmelnikow merged commit e983f7b into badges:master Nov 5, 2018
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants