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

Write documentation in Markdown using MyST #1089

Merged
merged 13 commits into from
Feb 8, 2022
Merged

Write documentation in Markdown using MyST #1089

merged 13 commits into from
Feb 8, 2022

Conversation

cauliyang
Copy link
Contributor

Reason

I am a fresh developer. This template is awesome and helps me a lot. Also, related blogs enable me to develop my own library and know more about how to build a library. However, I found that readme is in RST format, which may cause some inconvenience when I would like to insert an image and change size or position. I also try using .. image:: to change image position but it does not work in Github. What is more, .. raw:: is not supported by Pypi so that I cannot use HTML style. I know myst may fix the problem. But I only would like to change the readme file. Hence, I change some code in order to provide an option to choose what is the format of the readme. I am sure that there are some people who have the same needs as me.

I also test current code and run all nox sessions, there is no error. However, I do not know my idea is good and whether the code is compatible totally. :)

Copy link
Owner

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! Adding MyST to this project template has been on my mind for a while (#565). So it's good to see this spelled out for the README. Nice work!

Unfortunately, creating a template variable to control the format of the README is not an approach I want to take for this template. There are two reasons for this:

  1. I'm very reluctant to add configuration options to the template, generally. I like there to be only one way to use this template, where possible. If only because that keeps it more maintainable and testable for myself.
  2. When we switch documentation to Markdown, I would prefer all of it to switch, not just the README. Sadly, that's currently not possible at all, because docstrings still have to be written in RST. (I might compromise on this point though.)

So that means that I cannot accept this PR. I would consider a PR that works on the much larger task of switching everything to MyST. If you're willing to go down this road, let's talk it over carefully though beforehand.

I hope you understand, and thanks again for taking the time to work on this!

Update: As discussed in #565 , I'm happy to merge this PR if the RST option is removed and the README is always in Markdown.

@cauliyang
Copy link
Contributor Author

cauliyang commented Dec 6, 2021

Many thanks for your review! Myst is a good tool and can use the power of both rst and markdown at the same time. I only changed the readme for two reasons:

  1. Learning a new tool may take more time and result in unexpected errors. Some people may want their projects to look better by using a relatively short time frame.

  2. The readme of the project can be customized and powered by the property of markdown. As for other document files (usage, reference, etc), they still use rst because docstrings is written in the rst` style. This also considers the time cost.

However, I totally agree with your options as adding a new configuration may break the habit of using it. That also damages maintainability. Also, my change is not "clean" and perfect.

I have a question about what you mean by "switching everything to MyST", including docstring? MyST appears to be capable of using autodoc. This may allow us to write docstrings with rst or others (google style, numpy) and write document files (reference, usage, index) with markdown. MyST's this issue discusses writing docstrings in markdown.However, I think writing docstrings with rst or others (google style, numpy) may be good and look better. 

I am very willing to contribute something to this repo as it helps me a lot! If you think my option is not on the right track, please re-correct me! I like learning new things! :)

@cjolowicz
Copy link
Owner

I have a question about what you mean by "switching everything to MyST", including docstring?

I mean converting all reStructuredText documentation to Markdown, using MyST. Docstrings would have to stay in reStructuredText for now. But I guess it's ok.

@cauliyang
Copy link
Contributor Author

Thanks for your response , and that sounds so great 👍 . I will try to do that and hope to help the repo.

@cjolowicz
Copy link
Owner

Sure, thanks for helping out! Let me know if you have any questions. Feel free to open a draft PR anytime you feel like it.

@cjolowicz
Copy link
Owner

By the way, you can also open your PR in the instance repo: https://github.com/cjolowicz/cookiecutter-hypermodern-python-instance

It will be imported into the project template on merge.

@cjolowicz cjolowicz linked an issue Jan 4, 2022 that may be closed by this pull request
@cauliyang
Copy link
Contributor Author

cauliyang commented Jan 9, 2022

Hi @cjolowicz,

I have finished the changing for files under "docs/" as well as README.md, CONTRIBUTING.md, CODE_OF_CONDUCT.md.
Also, the license files are converted to md.

I have tested the commit, all looks good except one:

When I test sphinx build doc

image

this issue said that this may be fixed when documents are hosted in the cloud(readthedocs) instead of the local environment.

I will also create an instance and open PR to the instance repo later: https://github.com/cjolowicz/cookiecutter-hypermodern-python-instance if there is no other problem

Anyway, I will still work on this error. What do you think?

@pauleveritt
Copy link

Hi @cauliyang I'm very interested in this topic. I talked with @cjolowicz and offered to do the review. Is that ok? He gave me some of the things he's looking for before merging.

@cauliyang
Copy link
Contributor Author

Hi @pauleveritt ,

Sorry for the late reply, and I now try to create a test instance to check if myst reference target not found still exists when uploading to readthedoc. I am very happy to talk with you about this change^^

What do you think about what I submit now? Is there something wrong?

fix myst reference target failed error
@cauliyang
Copy link
Contributor Author

Hi @pauleveritt,

I have fixed the error about myst target according to changes of @paw-lu 's repo. Now the document system of md works well.

Do you think where I can improve to make that better?

@pauleveritt
Copy link

Sorry, I should have put in the bullet points from @cjolowicz as soon as we talked last week, rather than make you wait. Here were his points:

  • Include original license text. Don't use the markdown-ification. For legal reasons, better to keep originals. Also lets us track "upstream" more closely, if it changes.
  • Code of Conduct. He said there is a Markdown version "upstream", rather than us doing the rst->md ourselves. (Again, tracking upstream.)
  • Links for all sites. He said "Have the links work for all sites (GitHub, PyPI, etc.) as the relative links break." I kind of know what he means, but not exactly. 😀

None of this prevents me from starting a review, though.

@paw-lu
Copy link
Contributor

paw-lu commented Jan 17, 2022

I have fixed the error about myst target according to changes of @paw-lu 's repo. Now the document system of md works well.

I'm glad it helped! Thanks a bunch @cauliyang for implimenting it!

Do you think where I can improve to make that better?

I'll let @pauleveritt take over the review to limit the noise here, but have you looked at my other suggestions above? Highlights:

  1. Revert syntax from bashconsole
  2. Remove prepending whitespace from console commands ‎‎‎‎‎‎‏‏‎ ‎‏‏‎ ‎‏‏‎ ‎‏‏‎ ‎$ nox --list-sessions$ nox --list-sessions
  3. Change empty github-only code blocks to comments ```github-only% github-only
  4. Remove comments and placeholders (#temp in .gitignore, placeholder links)
  5. Remove uneeded empty brackets for hyperlinks [link text][][link text]

Again, thanks a lot for helping out here!

@paw-lu
Copy link
Contributor

paw-lu commented Jan 17, 2022

  • Links for all sites. He said "Have the links work for all sites (GitHub, PyPI, etc.) as the relative links break." I kind of know what he means, but not exactly. 😀

Just to be clear here (for everyone), a relative link like this:

Check out [CONTRIBUTING.md](CONTRIBUTING.md) for general contribution guidelines.

would work correctly when rendining on GitHub, but would not work on the rendered the README on PyPI. I even have an example of one I never got around to fixing. Check out the CONTRIBUTING.md link on the very bottom of both of these:

It only works properly on GitHub.

We might run into related issues also when rendering the README.md content on Read the Docs, which is why @cjolowicz originally used end-before: github-only comments to swap out links to prevent breaking.

So we have three main locations our pages render:

  1. GitHub
  2. PyPI
  3. Read the Docs

And want to make sure hyperlinks work as expected across all.


Also while you're here, I really enjoyed your static site talk @pauleveritt!

@cauliyang
Copy link
Contributor Author

Hi @pauleveritt,

Thanks for your recommendation, and I will try to follow the rules you mentioned here. Later, I will submit newest change.
😄

@cauliyang
Copy link
Contributor Author

Hi @paw-lu ,

it is so regretted that I miss your suggestions before, and I will go to read that carefully. Big thanks for your valuable review, and I will change these you mentioned as soon as possible.

I am happy to contribute this project and hope the project better 🚀

@paw-lu
Copy link
Contributor

paw-lu commented Jan 17, 2022

  • Include original license text. Don't use the markdown-ification. For legal reasons, better to keep originals. Also lets us track "upstream" more closely, if it changes.

To stay in sync with this template I also attempted to impliment this in a project generated from this repo. See PR if it helps:

@pauleveritt
Copy link

Thanks for the kind words @paw-lu and hope to see you more often around in this repo. Do you have time in 2022 to join in?

@cauliyang Do you think you can also handle the list from @paw-lu ?

@cauliyang
Copy link
Contributor Author

@pauleveritt, I will handle the list from @paw-lu as well. Hope we get a nice result!

yangli added 2 commits January 17, 2022 20:59
finished:

1. Include original license text
2. Code of Conduct
3. Links for all sites.
4. Revert syntax from bash → console
5. Remove prepending whitespace from console commands
6. Change empty github-only code blocks to comments github-only → % github-only
7. Remove comments and placeholders (#temp in .gitignore, placeholder links)
8. Remove empty brackets for hyperlinks [link text][] → [link text]
@cauliyang
Copy link
Contributor Author

cauliyang commented Jan 18, 2022

Finished:

@pauleveritt

  • 1. Include original license text
  • 2. Code of Conduct
  • 3. Links for all sites.

@paw-lu

  • 4. Revert syntax from bash → console
  • 5. Remove prepending whitespace from console commands
  • 6. Change empty github-only code blocks to comments github-only → % github-only
  • 7. Remove comments and placeholders (#temp in .gitignore, placeholder links)
  • 8. Remove empty brackets for hyperlinks [link text][] → [link text]

Do you think there is something error?

Update:

You can also check the toy instance, links for all sites work well.


This Code of Conduct is adapted from the `Contributor Covenant <homepage_>`__, version 2.0,
available at https://www.contributor-covenant.org/version/2/0/code_of_conduct.html.
This Code of Conduct is adapted from the [Contributor Covenant][], version 2.0,

Choose a reason for hiding this comment

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

Is the extra [] needed here (and in the next line)? I believe an earlier change flattened those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review again.

In fact, I have adopted the upstream version, and the current content is:

Attribution

This Code of Conduct is adapted from the [Contributor Covenant][homepage],
version 2.0, available at
[https://www.contributor-covenant.org/version/2/0/code_of_conduct.html][v2.0].

you can check this


.. _homepage: https://www.contributor-covenant.org
Community Impact Guidelines were inspired by [Mozilla’s code of conduct enforcement ladder][]
.

Choose a reason for hiding this comment

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

Just to be careful, let's get this period to the previous line (IMO).

Hmm, if this is an upstream version, then ignore this comment and the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as mentioned above,

current content in my forked repo:

Attribution

This Code of Conduct is adapted from the [Contributor Covenant][homepage],
version 2.0, available at
[https://www.contributor-covenant.org/version/2/0/code_of_conduct.html][v2.0].

Community Impact Guidelines were inspired by
[Mozilla's code of conduct enforcement ladder][Mozilla CoC].

You can check this

Maybe you see outdated file^ ^

@cjolowicz
Copy link
Owner

Thanks everybody for the great work here 💚

@cauliyang There seem to be some merge conflicts. Could you rebase your branch on main? (Or merge main into your branch, if you prefer.) Let me know if you have any questions about that.

@staticdev
Copy link
Contributor

I am just watching this PR with interest and also want to congratulate @cauliyang and everyone here.
Also want to mention @pauleveritt I am a big fan, watch hours and hours of your videos =)

@cauliyang
Copy link
Contributor Author

Hi @cjolowicz ,

I have merged main into my branch, and there are no conflicts now. Please feel free to check that ^ ^

Again, thanks for @paw-lu and @pauleveritt s' great help and review!

@pauleveritt
Copy link

Thanks @cauliyang for the work and @paw-lu for most of the useful input/

@staticdev Thanks for the kind word. I'm planning a big expansion this year on that front, would be interested in your input. Email me first dot last at company dot com.

Copy link
Contributor

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

@cauliyang thanks once again for the PR. There is still some tricky pre-commit issues on the generated project. This is why there are errors on Github Actions. To fix it, you need to generate a project using your repo:

cookiecutter --no-input gh:cauliyang/cookiecutter-hypermodern-python # generate a project using defaults
cd hypermodern-python && git init && git add --all
poetry install
nox -s pre-commit

This will autocorrect the formatting. Eg. you will see these generated files are missing the end of file:

  • CODE_OF_CONDUCT.md
  • README.md
  • docs/license.md
  • .gitignore
  • docs/requirements.txt
  • docs/contributing.md
  • docs/codeofconduct.md

Basically you can copy those files back in your repo and see the diff.

@cauliyang
Copy link
Contributor Author

Thanks, @staticdev, I have fixed all the problems preventing pre-commit passing the check.

@pauleveritt, I like Compare of Pycharm ❤️


[pull request]: https://github.com/cjolowicz/hypermodern-python/pulls

% github-only
Copy link
Contributor

Choose a reason for hiding this comment

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

This was my fault! I suggested this change, but it seems GitHub Flavored markdown does not support % commenting. My bad! We'll need to switch to something that GitHub, PyPI, and MyST respect.

Copy link
Contributor

@paw-lu paw-lu Jan 28, 2022

Choose a reason for hiding this comment

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

Seems <!-- --> is the officially documented syntax for GitHub Flavored Markdown comments. But does this work on MyST and PyPI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that GitHub cannot render % comments, and before I try to use

```github-only
```

But this will level a blank:

I will try <!-- --> later to see if it works for GitHub, PyPI, and myst

Copy link
Owner

Choose a reason for hiding this comment

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

Using <!-- --> should work. But you'll probably need to change the include directives in contributing.rst and index.rst to match the entire comment (<!-- github-only -->), not just the github-only part. Otherwise, docutils will include everything up to the <!--, turning the remaining document into a comment.

Copy link
Contributor Author

@cauliyang cauliyang Jan 28, 2022

Choose a reason for hiding this comment

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

Got it! Thanks, @cjolowicz. I will test for that

Copy link
Contributor

@eyllanesc eyllanesc Jan 28, 2022

Choose a reason for hiding this comment

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

Use <!-- github-only --> in README.md and

index.md

---
end-before: <!-- github-only -->
---

Copy link
Contributor Author

@cauliyang cauliyang Jan 28, 2022

Choose a reason for hiding this comment

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

Changed @paw-lu @cjolowicz

I have tested that, and it works well.

[documentation]: https://hypermodern-python.readthedocs.io/
[issue tracker]: https://github.com/cjolowicz/hypermodern-python/issues

# How to report a bug
Copy link
Owner

Choose a reason for hiding this comment

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

This heading needs to be second-level. Same for all headings after it.

Suggested change
# How to report a bug
## How to report a bug

This is important so the subheadings don't get displayed in the navigation sidebar in Sphinx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed: ab9b604

Tested result:
image

@cauliyang
Copy link
Contributor Author

cauliyang commented Feb 4, 2022

Hi @cjolowicz , I notice that there exist conflicts with version of sphnix-click:

<<<<<<< main
sphinx-click==3.0.2
myst_parser==0.16.1
=======
sphinx-click==3.1.0
>>>>>>> main

I can fix that as below:

sphinx-click==3.1.0
myst_parser==0.16.1

Does that work? I am concerned that I miss something

Updated:

merge into my branch, fix conflict and tested pre-commit

@eyllanesc
Copy link
Contributor

eyllanesc commented Feb 4, 2022

@cauliyang It should work, it seems that while you were implementing the PR the sphinx-click library was updated.

@@ -0,0 +1,65 @@
# {{ cookiecutter.friendly_name }}

[![pypi](https://img.shields.io/pypi/v/hypermodern-python.svg)](https://pypi.org/project/hypermodern-python/)
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the update @cauliyang

These images need to be templatized, there should be no "hypermodern-python".

Also it seems that the alt text is not the same as in the .rst version.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@cauliyang @cjolowicz

It was not only in README.md but in other files, I have created a PR adding those changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cjolowicz,

Thanks for your review 😄 !

That is my fault due to my carelessness. I have fixed that and tried to use an elegant way to change the alt text. I tested GitHub, PyPI, readthedocs as well. All works well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your help! @eyllanesc

You help me identify what is wrong. But you change 'title' instead of the alt text. Anyway, Thanks for your time and help:heart: !

Finished:

1. fix format for readme

2. fix trailing space for readme

3. fix duplicate reference link for pypi when build docs
@@ -1,3 +1,4 @@
furo=={{cookiecutter.copyright_year}}.1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be using template.. could you fix this also? It should be furo==2022.1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review! @staticdev , you can check here.
This is consistent with the current version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, I just noticed since you changed this file. But this is not intended, right @cjolowicz ?

Copy link
Owner

Choose a reason for hiding this comment

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

right, this is a version number and should not be templated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @staticdev, and @cjolowicz! It is fixed.

Copy link
Contributor

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

I rest my case XD

@cjolowicz cjolowicz added the documentation Improvements or additions to documentation label Feb 8, 2022
@cjolowicz cjolowicz linked an issue Feb 8, 2022 that may be closed by this pull request
@cjolowicz cjolowicz changed the title Add markdown readme option (enhancement) Write documentation in Markdown using MyST Feb 8, 2022
Copy link
Owner

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Huge thanks to @cauliyang and everybody who helped here @pauleveritt @paw-lu @staticdev @eyllanesc

@cjolowicz cjolowicz merged commit 69c9e26 into cjolowicz:main Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to the MyST documentation system
6 participants