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

Export wb_comment() #758

Merged
merged 8 commits into from Aug 23, 2023
Merged

Export wb_comment() #758

merged 8 commits into from Aug 23, 2023

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Aug 23, 2023

Proof of concept as discussed in #754

  • This does not change create_comment() in any way. It will continue working as it did before.
  • Change visible default to FALSE in wb_comment() to account for new Excel behaviour. (i.e. when I create a note in Excel, by default, it is not displayed)
  • Use package options for default author.
  • Allow a simple string to be passed to comment in wb_add_comment(). Most times, you just want to use the defaults.
  • I did the testing by opening in Excel. Seemed to work fine.
  • I did not add any deprecation message, but I can add it!

I will revamp examples and vignettes for that. So, I will be able to check if things are not working as expected.

I removed the default options to wbComment because it is not exported.
I completed the options list as some were missing from the list.

@olivroy olivroy marked this pull request as ready for review August 23, 2023 18:10
olivroy and others added 2 commits August 23, 2023 14:19
Merge commit 'fc908298037f085aff54c0b5bac78e90eed0d974'

#Conflicts:
#	R/class-comment.R
#	R/class-workbook-wrappers.R
#	_pkgdown.yml
#	man/comment.Rd
@JanMarvin
Copy link
Owner

After my initial complains ... I can live with this and we can merge prior to 1.0, aka I have no objection. Please test with threaded comments too.

@olivroy
Copy link
Collaborator Author

olivroy commented Aug 23, 2023

Should a deprecation warning be issued in create_comment() potentially only with the deprecation option?

@olivroy
Copy link
Collaborator Author

olivroy commented Aug 23, 2023

Threaded comments do not seem to use wbComment objects, if I am not mistaken?

@JanMarvin
Copy link
Owner

IIRC threaded comments use create_comment() internally. Even though it's a duplication I'd prefer to not yet deprecate create_comment() just a few days before the release. A deprecating soon option warning should be enough (similar to the camelCase one).

@olivroy
Copy link
Collaborator Author

olivroy commented Aug 23, 2023

Sounds good. Everything is ready. Deprecation is only if the option is set, like other ones. I will finalize the cleanup of create_comment() in a follow-up PR. (and look closely at threaded comments) (tests etc. ) so that I can close if I make an unintended change.

FYI, I plan to

  • add more @family tags so that the pkgdown index doesn't fail too much and to set the ordering with the functions, rather than manually. It was a complex exercise to try to know every function, so that's why the process was not smooth.
  • Remove the last write_formula/ write_* from examples and vignettes and test if the wb_add_formula alternative works in Excel. (test everything in excel obviously)

@JanMarvin JanMarvin merged commit 5d7668f into JanMarvin:main Aug 23, 2023
9 checks passed
@olivroy olivroy deleted the wb-comment branch August 23, 2023 20:24
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