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

contributing: add PR template #4207

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Conversation

petrasovaa
Copy link
Contributor

Based on discussion in #4197, I used the questions-answers template with small modifications.

I would like to keep the second level of headings to not have too giant font, but that interferes with the markdownlint style, @echoix any opinion?

@petrasovaa petrasovaa added this to the 8.5.0 milestone Aug 21, 2024
@petrasovaa petrasovaa self-assigned this Aug 21, 2024
@github-actions github-actions bot added CI Continuous integration docs labels Aug 21, 2024
veroandreo
veroandreo previously approved these changes Aug 21, 2024
@echoix
Copy link
Member

echoix commented Aug 21, 2024

For the linting error, we could just disable it. Take a look at the pull request template of Super-Linter: https://github.com/super-linter/super-linter/blob/0aa20740a5c28e959f0452846601e35a91d958c6/.github/pull_request-template.md

https://github.com/super-linter/super-linter/blob/0aa20740a5c28e959f0452846601e35a91d958c6/.github/pull_request-template.md?plain=1#L1-L10

They start with:

<!-- Start with an H2 because GitHub automatically adds the commit description before the template, -->
<!-- so contributors don't have to manually cut-paste the description after the H1. -->
<!-- Also, include the header in a "prettier ignore" block because it adds a blank line -->
<!-- after the markdownlint-disable-next-line directive, making it useless. -->
<!-- Ref: https://github.com/prettier/prettier/issues/14350 -->
<!-- Ref: https://github.com/prettier/prettier/issues/10128 -->
<!-- prettier-ignore-start -->
<!-- markdownlint-disable-next-line MD041 -->
## Readiness checklist
<!-- prettier-ignore-end -->

In order to have this pull request merged, complete the following tasks.

@wenzeslaus
Copy link
Member

This is very distracting in a PR template. Can this be disable in a configuration file for this specific file.

I think the H1 warning makes sense for most files, but not here, and having another disable to make the first disable work just adds to the confusion here.

Partially off-topic, but relevant: Now when I think about this, for Markdown documentation, we also expect to start with H2 just like we do in HTML because the file is a stub, not a complete file. We can't have this huge block in every documentation file, so global disable might be the right thing to do. (Although for other files, I think H1 is a good practice.)

@echoix
Copy link
Member

echoix commented Aug 22, 2024

Partially off-topic, but relevant: Now when I think about this, for Markdown documentation, we also expect to start with H2 just like we do in HTML because the file is a stub, not a complete file. We can't have this huge block in every documentation file, so global disable might be the right thing to do. (Although for other files, I think H1 is a good practice.)

Why wouldn't the new docs be complete files by themselves? (That would still have a parameters section injected)

@wenzeslaus
Copy link
Member

...
Why wouldn't the new docs be complete files by themselves?

Moved to a discussion: #4211

Co-authored-by: Markus Neteler <neteler@osgeo.org>
@petrasovaa petrasovaa enabled auto-merge (squash) August 26, 2024 13:45
@petrasovaa petrasovaa merged commit 22b07ac into OSGeo:main Aug 26, 2024
27 checks passed
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Sep 5, 2024
Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
echoix pushed a commit to echoix/grass that referenced this pull request Nov 30, 2024
@nilason
Copy link
Contributor

nilason commented Dec 2, 2024

Backported changes of .markdownlint.yml to branch 8.4.

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

Successfully merging this pull request may close these issues.

6 participants