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

Fix invalid rst format when alt or target is present #8275

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Fix invalid rst format when alt or target is present #8275

merged 4 commits into from
Aug 8, 2022

Conversation

abitrolly
Copy link
Contributor

No description provided.

@calebcartwright
Copy link
Member

Could you elaborate on the problem you're trying to solve with these proposed changes? If you have an example that reproduces the behavior that'd be great

@abitrolly
Copy link
Contributor Author

@calebcartwright I've looked for a test and found none, but I guess I could at least post the output, indeed.

The badge generated for fschulze/sqlalchemy_schemadisplay#31 was

.. image:: https://img.shields.io/pypi/v/sqlalchemy_schemadisplay   :alt: PyPI

And that is rendered as a missing image, because GitHub probably translates it to URL. The correct rST markup.

.. image:: https://img.shields.io/pypi/v/sqlalchemy_schemadisplay
   :alt: PyPI

@chris48s chris48s closed this Aug 7, 2022
@chris48s chris48s reopened this Aug 7, 2022
@shields-ci
Copy link

shields-ci commented Aug 7, 2022

Messages
📖 ✨ Thanks for your contribution to Shields, @abitrolly!

Generated by 🚫 dangerJS against 9ead774

@chris48s
Copy link
Member

chris48s commented Aug 7, 2022

@calebcartwright This PR does fix a bug. See https://docutils.sourceforge.io/docs/ref/rst/directives.html#image for the correct RST syntax.

@abitrolly The tests for this are in

test(reStructuredText, () => {
given('https://img.shields.io/badge', undefined, undefined).expect(
'.. image:: https://img.shields.io/badge'
)
given('https://img.shields.io/badge', undefined, 'Example').expect(
'.. image:: https://img.shields.io/badge :alt: Example'
)
given(
'https://img.shields.io/badge',
'https://example.com/example',
'Example'
).expect(
'.. image:: https://img.shields.io/badge :alt: Example :target: https://example.com/example'
)
})
and will need updating

I've closed and re-opened this to get the CI to run. We do have an issue where sometimes the Circle CI builds don't run for particular PRs - there is some info on this at #8109 I dunno if any of the info in #8109 (comment) are actionable for you. If not, we'll have to do the same again.

@abitrolly
Copy link
Contributor Author

The .spec.ts extension didn't trigger me that it is a file with tests. Looked like some alien typescript thing to me. Or maybe I was browsing my fork, which was too old to include this file.

Anyway, I think I've fixed tests.

@abitrolly
Copy link
Contributor Author

Regarding unblocking CircleCI, I don't see this as acceptable for my repos.

image

@chris48s chris48s closed this Aug 8, 2022
@chris48s chris48s reopened this Aug 8, 2022
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.

Merging this having pushed 5260a08 - Lets just merge the fix than do another round of back-and-forth over whitespace. Thanks

@chris48s chris48s added frontend The React app and the infrastructure that supports it bug Bugs in badges and the frontend labels Aug 8, 2022
@repo-ranger repo-ranger bot merged commit dbf94df into badges:master Aug 8, 2022
@abitrolly
Copy link
Contributor Author

Forgot \n at the ends 🤦 but seems that wouldn't satisfy the linter anyways.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs in badges and the frontend frontend The React app and the infrastructure that supports it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants