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

[comment] export wb_get_comment() #956

Merged
merged 2 commits into from Feb 24, 2024
Merged

[comment] export wb_get_comment() #956

merged 2 commits into from Feb 24, 2024

Conversation

JanMarvin
Copy link
Owner

@JanMarvin JanMarvin commented Feb 21, 2024

Why does my code not run?

Oh, there already is a function called wb_get_comment().

  • export wb_get_thread() too

Also ...

  • wb_comment() could be an S3 class, no need for the R6
  • I'd like to have something styled, but most ideas are to heavy. Either with third party packages like cli or javascript to create chart bubbles

@olivroy
Copy link
Collaborator

olivroy commented Feb 22, 2024

Last week, there was an existing workbook with comments and links in it. I couldn't copy the link from Excel. So I did what a normal person would do : load it in R.

and wb$comments became my friend at this moment. So I could see every comment and copy the Urls.

So my suggestion is:

Maybe if dims = NULL, this could error and point to the locations of comments in the sheet?

@JanMarvin
Copy link
Owner Author

Hi @olivroy , I picked wb_get_comment() to be in line with wb_add_comment(), but an additional s won’t do any harm I guess.

Could you elaborate the dims = NULL part? Right now all comments from a sheet are selected. And with dims this can be reduced to a selected region of a sheet. Why should we throw an error?

@olivroy
Copy link
Collaborator

olivroy commented Feb 22, 2024

Ah okay! Sweet. I misread the code, sorry 😞

@JanMarvin
Copy link
Owner Author

No worries, happens to me all the time 😄 But I’m not 100 percent sure I like to return everything as a data frame. It makes sense for the internal usage, but users probably would expect something nicer. Maybe I should just ditch the wbComment class and replace it with something easier to digest. I'd like some more styling, but haven’t made up my mind. I’ll think about it a bit more.

@JanMarvin JanMarvin merged commit 75a1c48 into main Feb 24, 2024
9 checks passed
@JanMarvin JanMarvin deleted the gh_issue_951 branch February 24, 2024 10:17
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