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: Convert api/*.rst -> api/*.md via rst2myst #2035

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Conversation

rht
Copy link
Contributor

@rht rht commented Feb 16, 2024

This has been brought up in dev meetings for a while, to have 1 format for documentation everywhere, not a mixture of .rst and .md. This is actually a long overdue change; @tpike3 could confirm this. The secondary reason is I want to try ChatGPT in making the docs clearer and more concise. See #2037.

@rht rht added the docs Release notes label label Feb 16, 2024
@Corvince
Copy link
Contributor

You opened several similar PRs. None of the gives any reasoning to the change. My comment relates to all of those. What issue does this address? What benefits does this provide? Its quite tedious work to review this, so what reasoning am I missing?

@rht
Copy link
Contributor Author

rht commented Feb 16, 2024

This has been brought up in dev meetings for a while, to have 1 format for documentation everywhere, not a mixture of .rst and .md. This is actually a long overdue change; @tpike3 could confirm this. The secondary reason is I want to try ChatGPT in making the docs clearer and more concise. See #2037.

I made several orthogonal PR's so that they can be reviewed piecemeal, and I have self-reviewed each myself. Where manual fixes are applied in the subsequent commits.

@Corvince
Copy link
Contributor

Ok good, thanks for the reply. Would be good to include something like that in at least one of the PRs as a description in the future.

@EwoutH
Copy link
Contributor

EwoutH commented Feb 16, 2024

@rht, please always add a PR description. Not only for us as maintainers, but also for others looking around.

Structure for example it like this:

  • Motivation or problem it solves
  • Why it solves the problem this way
  • (optionally) how it solves the problem (if non-trivial)

It's really important for reviews and users and contributors the PRs are accessible and as easy as possible to understand.

This is not optional, however small the PR.

Copy link
Contributor

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Thanks!

@rht
Copy link
Contributor Author

rht commented Feb 16, 2024

Added description.

@rht
Copy link
Contributor Author

rht commented Feb 16, 2024

Required statuses must pass before merging

@EwoutH
Copy link
Contributor

EwoutH commented Feb 16, 2024

Required statuses must pass before merging

Another #1962 bus factor issue!

@tpike3 or @jackiekazil could one of you remove all the required status checks? Approval by maintainers is already needed, we can interpret ourselves if the CI does what we want or not. We don't need a hard check for that.

@jackiekazil
Copy link
Member

@EwoutH -
Do you have this option? or is this only admins?

Merge without waiting for requirements to be met (bypass branch protections)

@Corvince
Copy link
Contributor

@EwoutH -
Do you have this option? or is this only admins?

Merge without waiting for requirements to be met (bypass branch protections)

This is only visible for admins.

Just for reference I added the required checks in response to #1788 (comment)
Sorry that this was done a bit haphazardly

But I agree we should remove them again.

@tpike3
Copy link
Contributor

tpike3 commented Feb 20, 2024

Required statuses must pass before merging

Another #1962 bus factor issue!

@tpike3 or @jackiekazil could one of you remove all the required status checks? Approval by maintainers is already needed, we can interpret ourselves if the CI does what we want or not. We don't need a hard check for that.

Disabled the pass status checks, I will not merge these ones to make sure there are not issues.

Copy link
Contributor

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Tested and look good to me! Thank you both!

@rht rht merged commit a83b8d3 into projectmesa:main Feb 21, 2024
3 checks passed
@rht rht deleted the docs3 branch February 21, 2024 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants