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

Create best practise guidelines #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Create best practise guidelines #2

wants to merge 5 commits into from

Conversation

sgetalbo
Copy link

Best practise document for new and existing packages - issue #1

@stephlocke
Copy link

@thibautjombart @zkamvar Can you review &/| approve please?

@thibautjombart
Copy link
Contributor

On it.

Copy link
Contributor

@thibautjombart thibautjombart left a comment

Choose a reason for hiding this comment

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

Great, thanks for this! Only a couple of comments and typos.

# Package contents
## README
- Ensure the use of non-RECON user friendly language, try not to refer to internal processes etc.
- Incude a clearly defined workflow and justification for using the package
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Include

- Internal functions should be tagged as so
- Commented out code should be removed to improve readability.
- All functions both internal and external should have roxygen documentation
- As a minimum internal functions should include a context e.g. #' This function is designed to...
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: one white space too many

- Functions should be clearly commented
- Internal functions should be tagged as so
- Commented out code should be removed to improve readability.
- All functions both internal and external should have roxygen documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

For internal function, can you precise if you mean full roxygen doc (e.g. incl examples), or just @param ?

- All functions both internal and external should have roxygen documentation
- As a minimum internal functions should include a context e.g. #' This function is designed to...
- Avoid using personal/colloquial language when writing comments and context - non-RECON users need to understand too.
- Examples for functions should appear in the roxygen comments rather than the README
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to understand: are you discouraging worked examples from the README? I thought these were useful for people who wanted to get an idea of the package's main features at a glance, but happy to change.

## Quality checks
Use packages like {devtools}, {lintr}, and {goodpractice} to ensure your code passes quality checks, correct spellings, and meets common formatting standards.

```r
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! :)

- Examples for functions should appear in the roxygen comments rather than the README

## Tests
- Tests should be written prior to the code.
Copy link
Contributor

Choose a reason for hiding this comment

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

If easy, could you provide a link to some basics principles of TDD?

Copy link

@stephlocke stephlocke left a comment

Choose a reason for hiding this comment

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

Made changes per @thibautjombart's comments

devtools::spell_check()
```

# Collaboration
Copy link
Member

Choose a reason for hiding this comment

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

I believe a section on branch protection is missing, so I've taken the liberty to add it here:

Suggested change
# Collaboration
# Collaboration
## Branches
The master branch should represent a stable state in the repository and should be protected from direct pushes and force-pushes using GitHub's [branch protection rules](https://help.github.com/en/github/administering-a-repository/about-protected-branches). Contributions to the master branch should only be made via Pull Requests with the exception of the initial few commits before user testing.
Pull requests should pass quality checks from continuous integration before merging.

@zkamvar
Copy link
Member

zkamvar commented Feb 27, 2020

One more thing regarding this: it should mesh with the guidelines found on the website: https://www.repidemicsconsortium.org/resources/guidelines/

@stephlocke
Copy link

@zkamvar incorporated 👍

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.

4 participants