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

[PHP-Eye] Get tested PHP version from PHP-Eye #1372

Merged
merged 11 commits into from
Dec 28, 2017
Merged

[PHP-Eye] Get tested PHP version from PHP-Eye #1372

merged 11 commits into from
Dec 28, 2017

Conversation

peter-gribanov
Copy link
Contributor

@peter-gribanov peter-gribanov commented Dec 14, 2017

For issue #819

In this PR i add badge for get PHP version only from PHP-Eye.

Badge for Symfony 2.8.0

PHP tested

Badge for Symfony 4.0.0

PHP tested

Attention

PHP-Eye does not return data about PHP 7.2, so it's best to check everything yourself.

@shields-ci
Copy link

shields-ci commented Dec 14, 2017

Warnings
⚠️

This PR modified helper functions in lib/ but not accompanying tests. That's okay so long as it's refactoring existing code.

Messages
📖

✨ Thanks for your contribution to Shields, @peter-gribanov!

📖

We ❤️ our documentarians!

Generated by 🚫 dangerJS

@peter-gribanov peter-gribanov changed the title Get tested PHP version from PHP-Eye [PHP-Eye] Get tested PHP version from PHP-Eye Dec 14, 2017
@peter-gribanov
Copy link
Contributor Author

peter-gribanov commented Dec 15, 2017

summary table

@peter-gribanov
Copy link
Contributor Author

Build failed because to exceeding GitHub limit.

@paulmelnikow
Copy link
Member

Could the left side label be PHP tested to be more consistent with our style? PHP would be okay too, though may not be as clear as you'd like.

Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

Looks good to me!

previewUri: '/php-eye/symfony/symfony.svg',
keywords: [
'PHP',
'php-eye'
Copy link
Member

Choose a reason for hiding this comment

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

Since these keywords are included in the title, there's no need to list them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And why then did we mention them here?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it was missed. Those could be removed too. Might be good to automate this check in a unit test.


t.create('gets the package version of symfony')
.get('/symfony/symfony.json')
.expectJSON({ name: 'Tested', value: '7.1' });
Copy link
Member

Choose a reason for hiding this comment

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

It's good that we have a test for the badge without a version because it ensures that code path is exercised. However I'm concerned this test will be brittle. Could you use a regex here instead of a constant '7.1'?

@paulmelnikow paulmelnikow added the service-badge New or updated service badge label Dec 20, 2017
@peter-gribanov
Copy link
Contributor Author

@paulmelnikow i fix all comments.

@paulmelnikow paulmelnikow merged commit f719705 into badges:master Dec 28, 2017
@paulmelnikow
Copy link
Member

Thanks!

@peter-gribanov peter-gribanov deleted the php-eye branch December 29, 2017 05:00
paulmelnikow added a commit that referenced this pull request Dec 5, 2018
See failures: https://circleci.com/gh/badges/shields/26998

It looks like the bug is in the original regex: #1372.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants