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

Added function to update rows #9

Merged
merged 8 commits into from
Jan 2, 2024

Conversation

bleonard314
Copy link

Added function to update rows based on the Smartsheet API documents. This function has the ability to match dataframe column names with Smartsheet column ids which may be useful for other functions. I also added '.Renviron' to the '.gitignore' to store project specific API keys (for dev only).

@cole-johanson
Copy link
Owner

@bleonard314 I'm a fan of this change! I like that you can update specific cells without having to specify the whole row. This actually makes me think this might be better named as ss_update_sheet() to coincide with ss_replace_sheet()? Or ss_update_cells()?

I do see a couple issues that need fixing:

  1. I wasn't able to run your example because length(row_numbers) != nrow(data). row_numbers = NULL by default, so this will abort the function. I think the solution is just to use if(!is.null(row_numbers) && length(row_numbers) != nrow(data)).
  2. row_offset does not have an effect with row_numbers. I think there are two solutions here. You could a) document this or b) remove row_offset and provide an example of how the user would accomplish this. Personally I prefer b. It would simply be setting row_numbers = row_offset:nrow(data), right? It seems simple enough that it does not require an additional argument.

Also, I prefer to use rlang::abort() instead of stop(), if you wouldn't mind updating that. Thanks!

@bleonard314
Copy link
Author

Thank you for the prompt review. I've implemented the suggested changes and enhanced the function with additional error checking for improved robustness. Additionally, the documentation now includes an example demonstrating the use of row_numbers for specifying an offset, aligning with your suggestion to minimize optional arguments. Really good idea!

Regarding the function naming, I've aligned it with the corresponding API method, "Update Rows". This decision is based on its direct interaction with the rows path, distinct from the sheets path. You can refer to the Smartsheet API documentation here for more details: Update Rows API.

Although there is an update method for sheets, it primarily addresses updates to an individual user's sheet settings, not the content itself. Hence, I believe renaming the function to ss_update_sheet() could lead to confusion. The alternative, ss_update_cells(), is a consideration, but maintaining consistency with the API's method names, as documented in the Smartsheet API and used in official repositories like the Smartsheet Python SDK, seems more intuitive and user-friendly. Here's the link to the SDK for reference: Smartsheet Python SDK.

I'm open to further suggestions or discussions on this matter.

@cole-johanson cole-johanson changed the base branch from main to develop January 2, 2024 15:05
Copy link
Owner

@cole-johanson cole-johanson left a comment

Choose a reason for hiding this comment

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

@bleonard314 thank you! This looks great. I am putting this in the develop branch. I have another change I hope to release this month.

@cole-johanson cole-johanson merged commit d23c40f into cole-johanson:develop Jan 2, 2024
1 check failed
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