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

feature/#37-named-by-sheet #146

Closed
wants to merge 84 commits into from
Closed

Conversation

spoltier
Copy link
Member

@spoltier spoltier commented Aug 19, 2021

Closes #37 - see miraisolutions/xlconnect-java#10

Sorry about the bad diffs, it was a while ago and my rstudio setup was probably not quite right

@spoltier

This comment was marked as resolved.

@spoltier

This comment was marked as resolved.

@riccardoporreca
Copy link
Member

riccardoporreca commented Sep 21, 2021

@spoltier, regarding the worksheet scope vs reference, I guess we can confirm the target is to introduce support for worksheet-scoped names.

Besides some specific technical / consistency feedback (coming soon), here some general inputs from my side

  • The argument is currently called worksheetName. Shall we be more explicit about its meaning by calling it worksheetScope? Otherwise, if we don't need to be specific w.r.t. to the argument being the name scope, we can perhaps just use sheet, which is what we use as argument in most of the existing XLConnect functions requiring a worksheet.

  • I would recommend defining the default argument of the user-exposed R function NULL instead of .jnull(class = "java/lang/String"), which is more natural and can better support programming around the XLConnect functions.

    • We can have an internal utility resolving R's NULL to Java's within each function at the time of calling the Java interface; perhaps even defining the quite common infix operator %||% and using it inline as worksheetName %||% .jnull(class = "java/lang/String"), see e.g. in rlang.
  • With reading data from a named region in mind, I think we need to support three cases

    • A) Same behavior as before this change if no / NULL argument specified, also for backwards compatibility: the "first" defined name is used.
    • B) Explicit sheet-scoped name, which is pretty much what is implemented now with the new argument.
    • C) Explicit workbook-scoped name: this is missing, meaning we do not provide any way to read a workbook-scoped name explicitly, unless it is (by chance?) the name retrieved as the "first" in case (A). => We could implement this as worksheetScope = FALSE: How to best propagate this to the Java side with a String argument is of course a question, perhaps just "" converted to sheet index -1 (global) internally.
      • UPDATE: Actually, it would be ideal to have this case covered by a string already in R (like ""), given that functions are meant to be vectorized and one may want to write in one go multiple named regions with a mix of sheet- and workbook-scoped names.

@riccardoporreca

This comment was marked as resolved.

R/xlcAttributesCall.R Outdated Show resolved Hide resolved
@spoltier spoltier force-pushed the feature/#37-named-by-sheet branch 2 times, most recently from 3ac8ba5 to c65a0c1 Compare January 17, 2024 09:48
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.

readNamedRegion and Local (sheet scoped) Names ?
2 participants