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.

Sign up for GitHub

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

Jump to bottom

Read named by sheet #10

Closed
wants to merge 36 commits into from
Closed

Read named by sheet #10

wants to merge 36 commits into from

Conversation

spoltier
Copy link
Member

@spoltier spoltier commented Aug 19, 2021 •
edited
Loading

@spoltier spoltier requested a review from martinstuder August 19, 2021 13:47
@spoltier spoltier self-assigned this Aug 19, 2021
@spoltier spoltier mentioned this pull request Sep 14, 2021
Closed
riccardoporreca
riccardoporreca requested changes Sep 23, 2021
View reviewed changes
Copy link
Member

@riccardoporreca riccardoporreca left a comment

Choose a reason for hiding this comment

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

@spoltier, see my comments and feedback.

On a general note, it might be good to have more consistency in terms of order of the additional argument, e.g. existsName(String name, String worksheetScope) vs. getReferenceFormula(String worksheetScope, String name). It seems more natural to add it after the name argument.

src/main/java/com/miraisolutions/xlconnect/Workbook.java Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
riccardoporreca
riccardoporreca requested changes Sep 23, 2021
View reviewed changes
Copy link
Member

@riccardoporreca riccardoporreca left a comment

Choose a reason for hiding this comment

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

@spoltier, some small refinements from a second pass

src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/Workbook.java Show resolved Hide resolved
src/main/java/com/miraisolutions/xlconnect/integration/r/RWorkbookWrapper.java Outdated Show resolved Hide resolved
martinstuder
martinstuder approved these changes Aug 9, 2022
View reviewed changes
src/main/java/com/miraisolutions/xlconnect/Workbook.java
if(validOnly && !isValidNamedRegion(namedRegion)) continue;
nameNames.add(namedRegion.getNameName());
}
public String[] getDefinedNames(boolean validOnly, String worksheetScope) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess there is still a "global"/"any" scope?

Copy link
Member

Choose a reason for hiding this comment

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

Given some details below I infer that worksheetScope = null is the "globa"/"any" scope ...

Copy link
Member Author

Choose a reason for hiding this comment

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

clarification (see miraisolutions/xlconnect#146 (comment) on the R side): worksheetScope = null means any, worksheetScope = "" means global scope only

src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated
public void createName(String name, String formula, boolean overwrite) {
if(existsName(name)) {

public void createName(String name, String worksheetScope, String formula, boolean overwrite) {
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken, the formula may include the worksheet name, in which case worksheetScope would be redundant?

Copy link
Member Author

@spoltier spoltier Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

formula may target a specific sheet, but the scope in which the name is defined (i.e. can be referenced) would be defined in worksheetScope. This is at least feasible manually in libreoffice:
image

src/main/java/com/miraisolutions/xlconnect/Workbook.java Outdated Show resolved Hide resolved
@spoltier spoltier force-pushed the feature/#37-named-by-sheet branch 3 times, most recently from 77b6b22 to 5f0e396 Compare January 17, 2024 09:14
spoltier and others added 19 commits February 14, 2024 13:05
@spoltier
xlconnect/#37 - named regions by name and filter by sheet name
804948f
@spoltier
xlconnect/#37 - WIP reference coordinates by worksheet
602868f
@spoltier
XLConnect #37 - better error message, simpler getNames
d24e719
@spoltier
XLConnect #37 - write in specific sheet's named region
3e7b695
@spoltier
XLConnect #37 - create, check existence, and append to named region
02027bc
@spoltier
XLConnect #37 - simplified worksheet name null handling
5e85f96
@spoltier
XLConnect #37 - clear named region in specific worksheet
3fa1835
@spoltier
XLConnect #37 - set worksheet on name if present
3fd469f
@spoltier
#37 fix scope v formula - sheet property
c31c9b8
@spoltier
#37 fall back to globally scoped name
9941ef4
@spoltier
gh #37: copy names when cloning
cd7fc7f
@spoltier
#37 clean up cloning code
dee51cb
@spoltier
#37 strict matching of worksheet name
fa5808f
@spoltier
Review: parameter name change to '...scope'
63192d0
@spoltier
Review: add getDefinedNames
1874f8a
@spoltier
Review: use "" for explicit global scope
11b3bb0
@spoltier
Review: worksheet scope for ref coordinates
d293147
@spoltier @riccardoporreca
Apply suggestions from code review
5b09391 
Co-authored-by: Riccardo Porreca <[email protected]>
@spoltier
Review: suggested refactorings
a08607f
spoltier and others added 17 commits February 14, 2024 13:05
@spoltier
Review: addImage with scope
9b68c9f
@spoltier @riccardoporreca
Apply suggestions from code review
e3dda7d 
Co-authored-by: Riccardo Porreca <[email protected]>
@spoltier
specify scope when failed name creation
b42f0b7
@spoltier
WIP - classes for attributes
88b4468
@spoltier
WIP - classes for attributes
aef9d73
@spoltier
classes for attributes, first working draft
5c6ba9f
@spoltier
createName - handle null case in worksheet name display
58e07e3
@spoltier
Actual Worksheet scope even when null passed
a7bf4a4
@spoltier
WIP readWorksheetName
a400a9a
@spoltier
WIP readWorksheetName
028f0ad
@spoltier
attribute values are arrays
53b077e
@spoltier
clean up after rebase
3907e8f
@spoltier
existsName: exception as warning
0939923
@spoltier
align wrapper method signatures to order in R package
4acd6c5
@spoltier
refactor parameter in Workbook order to match RWorkbookWrapper
9ea9aff
@spoltier
adjust parameter order for createName
729bfb7
@spoltier
bump major version for added API argument
ae5c94f
@spoltier spoltier force-pushed the feature/#37-named-by-sheet branch from b27657f to ae5c94f Compare February 14, 2024 12:06
@spoltier spoltier closed this Feb 14, 2024
@spoltier spoltier mentioned this pull request Feb 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riccardoporreca riccardoporreca riccardoporreca requested changes

@martinstuder martinstuder martinstuder approved these changes

Assignees

@spoltier spoltier

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@spoltier @martinstuder @riccardoporreca