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: improve contribution guide #486

Merged
merged 3 commits into from
Dec 7, 2024

Conversation

dduportal
Copy link
Contributor

This PR follows-up the discussion in https://github.com/asciidoctor/asciidoctor-reveal.js/pull/539/files#r1870061983.

It tries to improve the documentation over contribution guide, as they were "non explicit" areas which only were inside my brain. Healthiness of such a project needs maintainers to write down knowledge: this is the goal here (to avoid project being gate kept or depending on only one person).

It aims at improving the contributing experience by correctly explaining the expected changes of contributions.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dduportal Damien Duportal
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
You need the following tools:

* A bash compliant command line
* link:http://man7.org/linux/man-pages/man1/make.1.html[GNU make]
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: link is optional and I like to extract URL at the top of the file:

:gnu-make-url: http://man7.org/linux/man-pages/man1/make.1.html

* {gnu-make-url}[GNU make]

Also, I think we should https URL

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 @ggrossetie ! I've added the https link.

About the URL on top of the file: what is the rationale behind? I tend to do it when I repeat the URL on the file at least twice, but there might a good practise here that I don't know

Regarding the optional link, I tend to it because we have a Markdown file (for use on the DockerHub) so I tend to be explicit to make it easier differentiating between both format.

Copy link
Member

Choose a reason for hiding this comment

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

I find it easier to update (potentially broken) links when they are all together at the top of the file. When in doubt, you can ctrl+click all of them since they are located at the top of your file and see if they are still working as expected. It's a bit more tedious when external links are scattered.

Regarding the optional link, I tend to it because we have a Markdown file (for use on the DockerHub) so I tend to be explicit to make it easier differentiating between both format.

👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense! If you don't mind, I'll proceed with this PR (as it is a nitpick) and I'll send a subsequent PR with:

  • replacing remaining http:// scheme by https:// (we're in 2024 so either these are dead links to remove or they can handle https:// scheme)
  • Moving the links on top of the adoc documentations as described here

CONTRIBUTING.adoc Outdated Show resolved Hide resolved
CONTRIBUTING.adoc Outdated Show resolved Hide resolved
dduportal and others added 2 commits December 7, 2024 15:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Guillaume Grossetie <ggrossetie@yuzutech.fr>

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dduportal Damien Duportal
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal dduportal requested a review from ggrossetie December 7, 2024 14:48
@dduportal dduportal merged commit 1cf2f4d into asciidoctor:main Dec 7, 2024
2 checks passed
@dduportal dduportal deleted the docs/contributing branch December 7, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants