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

docs: Update template README.rst to match OEP-55 Guidelines #241

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Aug 9, 2022

Description:

Update the README.rst to match guidance in OEP-55

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

@feanil
Copy link
Contributor Author

feanil commented Aug 9, 2022

Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I think that cookiecutter-xblock/{{cookiecutter.repo_name}}/README.rst is another templated README. Not sure if that is also on your list.


Overview (please modify)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there should be some replacement for "(please modify)". Could be something like:

.. note::

  This README was auto-generated. The "Purpose" needs to be manually updated, and the rest of the README should be reviewed for potential updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

@feanil: Can you confirm your intention? Not sure if you intentionally don't want to add a note, or if you just missed this. Thanks.

Also note that if you read the README, it is clear that it should be updated. But, I'm just wondering if anything can make it more clear for cases where your eyes are passing over it, but you aren't actually reading it. It might be that mu suggestion is no better than what is there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed this the first time, I'll add it along with the other suggestions.

@feanil feanil requested a review from robrap August 11, 2022 17:46
Base automatically changed from feanil/re-rule to master August 11, 2022 17:50
@feanil feanil force-pushed the feanil/update_readme branch from ffbf2ad to f0fddb9 Compare August 11, 2022 17:51
Copy link

@Carlos-Muniz Carlos-Muniz left a comment

Choose a reason for hiding this comment

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

After reading the README Spec sheet, this looks good! It would be great to have this as a standard everywhere.

How many more repos do we need to do this for? Do we have a ticket anywhere to update READMEs to fit this format, where we can track the repos that still need their READMEs updated?

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

A couple of suggestions, but otherwise, approved! 👍🏼

Deploying
=========

How can a new user, go about deploying this component? Is it just a few commands? Is there a larger how-to that should be linked here?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
How can a new user, go about deploying this component? Is it just a few commands? Is there a larger how-to that should be linked here?
How can a new user go about deploying this component? Is it just a few commands? Is there a larger how-to that should be linked here?


For details on how to deploy this component, checkout the `deployment how-to`_

.. _deployment how-to: https://docs.openedx.org/projects/this-project/how-tos/how-to-deploy-this-component.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe {{cookiecutter.repo_name}} in here, too?

Suggested change
.. _deployment how-to: https://docs.openedx.org/projects/this-project/how-tos/how-to-deploy-this-component.html
.. _deployment how-to: https://docs.openedx.org/projects/{{cookiecutter.repo_name}}/how-tos/how-to-deploy-this-component.html

Fix a typo and add a header to clearly indicate that the readme should
be read and updated after it has been generated.
@feanil feanil enabled auto-merge August 12, 2022 13:27
@feanil feanil merged commit f82bf92 into master Aug 12, 2022
@feanil feanil deleted the feanil/update_readme branch August 12, 2022 13:29

Documentation
*************
The ``README.rst`` file should start with a brief description of the repository and its purpose.
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 this is merged already, but thinking about linting, and also for clarification: what if we started each paragraph like this with PLACEHOLDER:, then linted for the appearance of that word in READMEs? It would help everyone (maintainer and readers of the file) understand that the paragraph is just a placeholder, leave a strong signal that something needs to be done, and be easily lintable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I will make a PR with this change, and we can discuss there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants