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

New function save_to_gh() #5

Open
wants to merge 13 commits into
base: DEV_v2
Choose a base branch
from
Open

New function save_to_gh() #5

wants to merge 13 commits into from

Conversation

RossanaTat
Copy link

This PR fixes and improves the old save_to_gh() function. The updated version builds on the original, making sure it works in all cases. The main issue was that it wouldn’t update the data of an existing file, but now it works for all scenarios:

  • Adding new data to a new file,
  • Updating an existing file with new data, and
  • Handling cases where the file already exists but the data is the same.
    Key improvements also include better handling of metadata and making sure it’s checked properly when it’s provided.

This PR also adds new tests and updates the vignette, specifically the "Saving Data to GitHub" section in interact_with_Github.Rmd.

R/save_to_gh.R Outdated
Comment on lines 56 to 64
if (!requireNamespace("gh", quietly = TRUE)) {
stop("Package 'gh' is required. Please install it using install.packages('gh').")
}

if (!requireNamespace("cli", quietly = TRUE)) {
install.packages("cli")
library(cli)
}

Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary because both gh and cli should be part of the namespace of this package. As long as these packages are in the imports section of the DESCRPTION file, you don't need to add these lines of code. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @randrescastaneda, thank you for reviewing. That's right, those lines are redundant because both cli and gh are in the Imports section. I removed them and pushed again.

Copy link
Member

@randrescastaneda randrescastaneda left a comment

Choose a reason for hiding this comment

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

Hi @RossanaTat, The changes look great, thank you so much. I added A comment about a few lines that I don't think should be there. but please let me know if you disagree. Otherwise, just. make the change and push again.

@dana89co
Copy link

Hi @RossanaTat,
Thank you so much!
I see that this function now fixes the problem with the 'metadata' error and the tests you created make more sense 👍 Let me know when you feel these changes are enough and I can try to merge this branch to mine (which has the other fixes).

@RossanaTat
Copy link
Author

Hi @randrescastaneda, I've addressed the changes you suggested and pushed again. Tests on the new save_to_gh() function are passing, however please note that check() still fails because of other issues we need to work on on different branches. Thank you!

Copy link
Member

@randrescastaneda randrescastaneda left a comment

Choose a reason for hiding this comment

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

Hi @RossanaTat,
Thank you. I checked the PR, and it seems to me that we need to fix the issues of the check() before merging. I ran it myself and there are many things that are failing. Could you please take care of the issues? If they are difficult to solve, please let me know and we find a way to fix them soom. Thanks.

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.

3 participants