-
Notifications
You must be signed in to change notification settings - Fork 57
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 readme render #607
Fix readme render #607
Conversation
Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
3 similar comments
Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
2a7ba2c
to
47a183a
Compare
47a183a
to
cb1addd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly fine but I'm concerned about the maintainability of the html-skipping part (see inline comment)
- $MAIN_CMD $SETUP_CMD | ||
- if [[ $TWINE_CHECK ]]; then | ||
python setup.py build sdist; | ||
twine check dist/*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is diabolical (in a good way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😈
lines = ff.read().splitlines() | ||
|
||
# Skip lines that contain raw HTML markup | ||
lines = lines[:4] + lines[26:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit dangerous - what if someone else makes the README and doesn't realize they need to update this file?
Maybe instead look for the .. raw:: html
marker, and then skip all consecutive following lines that are indented (or empty)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're absolutely right. We should do something smarter like that in the long term but for the sake of this bug fix I'm going to leave it alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the point of the twine check
above is that it should catch any such changes that lead to invalid output, but we're in agreement.
Good points re: the comment @drdavella - I'll make an issue right now to track the "make the html thingie work better", but doesn't have to go in this PR. |
This is intended to fix #605 and also to fix rendering of the README on pypi. It seems nearly impossible to test without another release, unfortunately.