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: Replace use of raw directive in README.rst #825

Merged
merged 5 commits into from
Apr 21, 2020

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Apr 19, 2020

Description

Resolves #824

In PR #819 the use of the raw directive in rST was used to render images correctly. However, this is not allowed on PyPI and breaks the publication checks.

ReadTheDocs build: https://pyhf.readthedocs.io/en/fix-fix-raw-html-in-readme.rst/

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Remove use of raw directive in README.rst as not allowed on PyPI
   - Amends PR #819
* Resize image files for Brazil Band plots in README to be 500 px wide
   - c.f. https://github.com/github/markup/issues/295
* Add twine check of dist to publishing CI tests

@matthewfeickert matthewfeickert added docs Documentation related CI CI systems, GitHub Actions fix A bug fix labels Apr 19, 2020
@matthewfeickert matthewfeickert self-assigned this Apr 19, 2020
@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #825 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #825   +/-   ##
=======================================
  Coverage   95.92%   95.92%           
=======================================
  Files          54       54           
  Lines        3016     3016           
  Branches      424      424           
=======================================
  Hits         2893     2893           
  Misses         78       78           
  Partials       45       45           
Flag Coverage Δ
#unittests 95.92% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaf15cb...997c7af. Read the comment docs.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Apr 19, 2020

@kratsg you had a suggestion here about 64 bit string encoding of images for the resize? Can you elaborate?

There is an example on the Stack Overflow question "Can you resize or change resolution of base64 image through manipulating the base64 code?" but I'm not sure how to be able to do this without editing the source images themselves as we can't use raw HTML or javascript on PyPI.

@kratsg
Copy link
Contributor

kratsg commented Apr 21, 2020

There is an example on the Stack Overflow question "Can you resize or change resolution of base64 image through manipulating the base64 code?" but I'm not sure how to be able to do this without editing the source images themselves as we can't use raw HTML or javascript on PyPI.

this is only for the SO tag. Can you just not resize the other images and save the resized images in the repo? Then we don't need to explicitly resize them. Or we get rid of the README on github and move the README into the docs, because GitHub is clearly the problem here...

@matthewfeickert
Copy link
Member Author

Can you just not resize the other images and save the resized images in the repo? Then we don't need to explicitly resize them.

This certainly is the easiest, but I wanted to avoid having to do this as then we have to downsize the logo, which is used in other places online from this repo and it would be nice to keep it large.

Or we get rid of the README on github and move the README into the docs, because GitHub is clearly the problem here...

I think it is better to have a README with weird sized images than no README in the top level of the repo at all. We could move the rST README into the docs and then have a Markdown README at the top level, but then we're maintaining two versions of the same file.

GitHub won't resize images in rST
@matthewfeickert
Copy link
Member Author

This certainly is the easiest, but I wanted to avoid having to do this as then we have to downsize the logo, which is used in other places online from this repo and it would be nice to keep it large.

I guess the only thing I really care about is the pyhf logo. To me, the only images in the GitHub render of the README that are offensive are the CLs plots. I've resized the ones that are the worst offenders to have widths of 500 pixels (I didn't bother with the one that is 514 pixels wide) using GIMP. If we can live with a slightly gaudily large pyhf logo and big Stack Overflow logo in the README then I think this fixes things.

@matthewfeickert matthewfeickert marked this pull request as ready for review April 21, 2020 08:34
@kratsg
Copy link
Contributor

kratsg commented Apr 21, 2020

This certainly is the easiest, but I wanted to avoid having to do this as then we have to downsize the logo, which is used in other places online from this repo and it would be nice to keep it large.

provide an svg version of the logo and a png version that's "logo-small.png" or similar -- this is probably better.

@kratsg
Copy link
Contributor

kratsg commented Apr 21, 2020

Screenshot 2020-04-21 08 17 28

checkboxes look consistent btw. The image should show as long as it's included in master.

@kratsg kratsg merged commit 5fa43bc into master Apr 21, 2020
@kratsg kratsg deleted the fix/fix-raw-html-in-README.rst branch April 21, 2020 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI systems, GitHub Actions docs Documentation related fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of raw directive in README.rst not allowed on PyPI
2 participants