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

Update function documentation #78

Merged
merged 14 commits into from
Mar 4, 2025
Merged

Conversation

joshwlambert
Copy link
Collaborator

This PR addresses #58 & #67.

I've removed duplicated documentation of function arguments and used @inheritParams instead. outbreak_model.R is the main function that documents arguments and other functions that have the same arguments inherit from there.

I've added @return documentation to exported functions that were previously missing it.

Issue #27 could also have been addressed in this PR, but as I'm still getting familiar with the codebase I didn't have many improvements on the general function documentation for now, and will address this in a subsequent PR.

@joshwlambert joshwlambert added the documentation Improvements or additions to documentation label Mar 3, 2025
@joshwlambert joshwlambert requested a review from sbfnk March 3, 2025 14:47
Copy link
Collaborator

@pearsonca pearsonca left a comment

Choose a reason for hiding this comment

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

Useful start; seems like assorted minor missing bits

Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

Just a couple small comments, thanks!

@joshwlambert joshwlambert mentioned this pull request Mar 3, 2025
@joshwlambert
Copy link
Collaborator Author

Although this PR wasn't targeting #27, the suggestions and changes have tackled some of the points mentioned in that issue. I've left a comment in #27 that only @examples are required, IMO, before it can be closed.

Thanks for the comments @pearsonca and @sbfnk, let me know if there's anything else to the changed, if not, I'm happy for this to be merged when you're ready.

Copy link
Collaborator

@pearsonca pearsonca left a comment

Choose a reason for hiding this comment

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

I think it's fine as is, but also commented on two minor improvements you might make

@sbfnk sbfnk enabled auto-merge March 4, 2025 10:00
@sbfnk sbfnk added this pull request to the merge queue Mar 4, 2025
Merged via the queue into epiforecasts:main with commit e7c8776 Mar 4, 2025
7 checks passed
This was referenced Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants