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

Documentation updates #41

Merged
merged 26 commits into from
Apr 3, 2021
Merged

Documentation updates #41

merged 26 commits into from
Apr 3, 2021

Conversation

jeffreyhanson
Copy link
Collaborator

This PR aims to help improve documentation. Specifically, it adds additional details and examples to the function documentation and adds a vignette (per #12). As discussed over email, I have also updated the DESCRIPTION to list myself as a contributor. To try and make this PR as easy as possible to review, I have made multiple commits related to specific changes. Is this actually helpful? If not, please let me know and I can squash all changes into a single commit.

@jeffreyhanson
Copy link
Collaborator Author

I wasn't sure exactly what information needed to be in the vignette, but hopefully it provides a useful starting point for new users? Let me know if there's additional topics/functionality that should be covered? Also, please feel free to edit/change/update any of the documentation in this PR if anything's not clear (or in case of typos)?

@jeffreyhanson
Copy link
Collaborator Author

jeffreyhanson commented Mar 5, 2021

Also, since this PR only covers documentation-related stuff, the Windows CI checks are still failing (per #34).

- add missing dep for covr pkg
@dirkschumacher
Copy link
Owner

Thank you so much 🙏. I will try to take a deeper look before merging.

@jeffreyhanson
Copy link
Collaborator Author

Ah - now we've got merge conflict issues due to the automated package website rebuild. Sorry about that - I'll take care of this now.

@jeffreyhanson
Copy link
Collaborator Author

jeffreyhanson commented Mar 6, 2021

After playing around some more with the rcbc, I realized that the CBC parameters should be in lower case to actually work. Currently, the documentation in this PR incorrectly provides the parameters in upper case. I'll update the documentation to address this mistake.

@jeffreyhanson
Copy link
Collaborator Author

Ah - I'm sorry, I just realized this PR has merge conflict issues. I'll try and address them now.

@jeffreyhanson
Copy link
Collaborator Author

I'm sorry, this PR has a lot of unnecessary commits. Is that an issue? Please let me know if you would like me to squash all this into a single commit, or open up a new PR (based on a new branch) with just the relevant changes copy-pasted from this branch?

.github/workflows/check-ubuntu.yaml Outdated Show resolved Hide resolved
vignettes/rcbc.Rmd Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
@dirkschumacher
Copy link
Owner

Really great stuff. Thanks for writing that and also adding some literature references.

@jeffreyhanson
Copy link
Collaborator Author

jeffreyhanson commented Apr 2, 2021

No worries - I'm happy to help! Thanks so much for making the time to look at this.

@jeffreyhanson
Copy link
Collaborator Author

Ah - I've introduced another merge conflict. I'll fix that now.

@jeffreyhanson
Copy link
Collaborator Author

jeffreyhanson commented Apr 2, 2021

Ok - I think I've addressed all the issues with this PR. @dirkschumacher, please let you know if there's any further I changes I can make to improve it?

@dirkschumacher dirkschumacher merged commit fc667fb into dirkschumacher:master Apr 3, 2021
@dirkschumacher
Copy link
Owner

Great thanks. LGTM!

@jeffreyhanson
Copy link
Collaborator Author

Awesome - thanks! I'll work on updating the README now and adding citation information (e.g. so the users are told to cite the rcbc package and also CBC when using this package).

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.

None yet

2 participants