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: add slightly tweaked boilerplate CONTRIBUTING.md #1423

Merged
merged 6 commits into from
Jul 4, 2024
Merged

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Jul 4, 2024

Fix #1422

cf #1421 (comment)

The CONTRIBUTING.md guide is where R developers would expect to find these things, especially as pkgdown will link to the file automatically.

@maelle maelle requested review from szhorvat and krlmlr July 4, 2024 07:38
Copy link
Contributor

aviator-app bot commented Jul 4, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

Comment on lines 5 to 10
## Fixing typos

You can fix typos, spelling mistakes, or grammatical errors in the documentation directly using the GitHub web interface, as long as the changes are made in the _source_ file.
This generally means you'll need to edit [roxygen2 comments](https://roxygen2.r-lib.org/articles/roxygen2.html) in an `.R`, not a `.Rd` file.
You can find the `.R` file that generates the `.Rd` by reading the comment in the first line.
Copy link
Member

Choose a reason for hiding this comment

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

Will people need to regenerate the .Rd file themselves (how?), or will this be done for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we'd do it for them for now

Comment on lines 18 to 33
### Pull request process

* Fork the package and clone onto your computer. If you haven't done this before, we recommend using `usethis::create_from_github("igraph/rigraph", fork = TRUE)`.

* Install all development dependencies with `pak::pak()`, and then make sure the package passes R CMD check by running `devtools::check()`.
If R CMD check doesn't pass cleanly, it's a good idea to ask for help before continuing.
* Create a Git branch for your pull request (PR). We recommend using `usethis::pr_init("brief-description-of-change")`.

* Make your changes, commit to git, and then create a PR by running `usethis::pr_push()`, and following the prompts in your browser.
The title of your PR should briefly describe the change.
The body of your PR should contain `Fixes #issue-number`.
Copy link
Member

@szhorvat szhorvat Jul 4, 2024

Choose a reason for hiding this comment

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

Are you sure that managing Git entirely through R packages is the easiest, most user friendly way? Personally I find this very confusing, and I wouldn't trust any automated solution to create a PR for me. I'd rather create my own PR: it's easy enough from GitHub's web interface.

But then I'm not coming from R.

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 tweaked the text! I actually use... another R package 😂 (gert)

Comment on lines 37 to 38
* We use [testthat](https://cran.r-project.org/package=testthat) for unit tests.
Contributions with test cases included are easier to accept.
Copy link
Member

Choose a reason for hiding this comment

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

Should tests be required for all new feature? (I think they should.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I changed the phrasing.

Copy link
Member

@szhorvat szhorvat left a comment

Choose a reason for hiding this comment

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

I added some comments, as you requested, and I hope they are helpful. I think these decisions should be made by @krlmlr @maelle @Antonov548 (the R team), so please take them as comments/suggestion, not requests. I am not familiar with R development customs.

Speaking of styling, let's make that consistent for Markdown as well. Either use no line breaks at all in paragraphs (this is what I do) or break everything at a consistent column number. Right now this .md file is a bit of a mess, formatting-wise 😃

@maelle
Copy link
Contributor Author

maelle commented Jul 4, 2024

@krlmlr @Antonov548 feel free to suggest changes, I'm going to go ahead and merge this as nothing looks controversial to me. 😇 (optimistic merging!)

@aviator-app aviator-app bot merged commit 4b99170 into main Jul 4, 2024
2 checks passed
@aviator-app aviator-app bot deleted the ctb branch July 4, 2024 11:44
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.

Document test/script alignment in CONTRIBUTING.md
2 participants