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

Static Code Climate examples #2201

Merged
merged 9 commits into from
Oct 23, 2018
Merged

Conversation

PyvesB
Copy link
Member

@PyvesB PyvesB commented Oct 20, 2018

We've hit a wall in #1518, our Code Climate badges have been unreliable for months now.

This pull request aims at sparing some of our limited API quota by switching to static examples on the homepage.

I'm not willing to migrate the badges to the new service architecture at this point as we've already put in quite a lot of investigation and development effort in these badges. The least we can say is that @codeclimate have not been very cooperative and have been dragging their feet since one of our users first contacted them back in April.

@PyvesB PyvesB added performance-improvement Related to performance or throughput of the badge servers service-badge New or updated service badge labels Oct 20, 2018
@shields-ci
Copy link

shields-ci commented Oct 20, 2018

Warnings
⚠️

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

Messages
📖

✨ Thanks for your contribution to Shields, @PyvesB!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2201 October 20, 2018 17:24 Inactive
@PyvesB
Copy link
Member Author

PyvesB commented Oct 20, 2018

I have deployed a review app, everything seems fine apart from the static examples not showing up. Am I being thick here? 😄

@chris48s
Copy link
Member

I think if you want to use staticExample in all-badge-examples.js, you need to define it as a static badge URL instead of just a javascript object because its not going though any of this:

shields/services/base.js

Lines 90 to 106 in 95f1b50

static _makeStaticExampleUrl(serviceData) {
const badgeData = this._makeBadgeData({}, serviceData)
const color = badgeData.colorscheme || badgeData.colorB
return this._makeStaticExampleUrlFromTextAndColor(
badgeData.text[0],
badgeData.text[1],
color
)
}
static _makeStaticExampleUrlFromTextAndColor(text1, text2, color) {
return `/badge/${encodeURIComponent(
text1.replace('-', '--')
)}-${encodeURIComponent(text2).replace('-', '--')}-${encodeURIComponent(
color
)}`
}

I've not checked if this works, but given LegacyService extends BaseService, you can probably move the examples out of all-badge-examples.js and into the service class without having to do a full refactor of the service class. You might also need to define url() in there so you've got a base..

@paulmelnikow paulmelnikow had a problem deploying to shields-staging-pr-2201 October 21, 2018 17:17 Failure
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2201 October 21, 2018 17:20 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2201 October 21, 2018 17:22 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2201 October 21, 2018 17:29 Inactive
@PyvesB PyvesB changed the title Static Code Climate examples WIP Static Code Climate examples Oct 21, 2018
@PyvesB
Copy link
Member Author

PyvesB commented Oct 21, 2018

@chris48s thank you for your help! This piece of work should now be ready for review.

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No probs. Approach looks good, but 3 of the examples look a bit odd because the colour doesn't match with the value on the badge.

staticExample: {
label: 'coverage',
message: '95%',
color: 'yellowgreen',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be green (or a lower value)

title: 'Code Climate coverage (letter)',
exampleUrl: 'coverage-letter/jekyll/jekyll',
urlPattern: 'coverage-letter/:userRepo',
staticExample: { label: 'coverage', message: 'A', color: 'green' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be brightgreen (or a lower value)

staticExample: {
label: 'technical debt',
message: '3%',
color: 'green',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also be brightgreen (or a lower value)

@PyvesB
Copy link
Member Author

PyvesB commented Oct 22, 2018

You're right, I got confused with the shades of green!

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2201 October 22, 2018 19:52 Inactive
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorted - cheers

@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:

@RedSparr0w
Copy link
Member

Nice work, hopefully this reduces the API hits a fair amount.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance-improvement Related to performance or throughput of the badge servers service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants