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

Adding Append support to Excel.Write #3558

Merged
merged 35 commits into from
Jul 7, 2022
Merged

Adding Append support to Excel.Write #3558

merged 35 commits into from
Jul 7, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jul 1, 2022 •
edited
Loading

Pull Request Description

Adds support for appending to an existing Excel table.

Important Notes

  • Renamed Column_Mapping to Column_Name_Mapping
  • Changed new type name to Map_Column
  • Added last modified time and creation time to File.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide dist and ./run ide watch.

@jdunkerley jdunkerley force-pushed the wip/jd/excel-write-182309816 branch from b85ad70 to 37d6a67 Compare July 1, 2022 13:08
@jdunkerley jdunkerley force-pushed the wip/jd/excel-append-182309853 branch from 6d0d619 to f8abb1b Compare July 1, 2022 13:08
@jdunkerley jdunkerley force-pushed the wip/jd/excel-write-182309816 branch from 4733e64 to 5e0a68e Compare July 4, 2022 15:58
@jdunkerley jdunkerley force-pushed the wip/jd/excel-append-182309853 branch from da29efe to 9d165d3 Compare July 4, 2022 16:19
Base automatically changed from wip/jd/excel-write-182309816 to develop July 4, 2022 18:32
jdunkerley and others added 20 commits July 5, 2022 10:39
@jdunkerley
Initial work for Excel write.
cd1dc4e 
- Added File.copy_to
- Removed writeSpreadsheetCell.
- Added ExcelWriter.
@jdunkerley
Close to working for a Sheet write.
08488f1 
Still to do Range based.
@jdunkerley
Excel Write work.
9038756
@jdunkerley
Expand the write data to support range based writing.
e80a819
@jdunkerley
Range writing
f470661 
Header Infer based on current
@jdunkerley
Handle errors.
86f0a6c
@jdunkerley
Some more tests.
7e64acf 
Fix issues with BLANK cells.
@jdunkerley
Lots of tests...
fd46fe0
@jdunkerley
Java Format.
f2ea4d3
@jdunkerley @radeusgd
Update distribution/lib/Standard/Table/0.0.0-dev/src/Io/Excel.enso
4a427f2 
Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
@jdunkerley @radeusgd
Update distribution/lib/Standard/Table/0.0.0-dev/src/Io/Excel.enso
4bb1aa4 
Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
@jdunkerley
Most PR comments.
482efc0
@jdunkerley
More PR comments.
af3db59
@jdunkerley
Rename method.
612cd77
@jdunkerley
Working on the Append mode.
737f4bd
@jdunkerley
Add Map_Columns type and pass to Java.
a7c74b4
@jdunkerley
Starting on Append.
b099733
@jdunkerley
Add some more existing file tests and restructure to allow for append.
9f9aadf
@jdunkerley
Remove getDefaultComparator
ac08233
@jdunkerley
Append implementation.
f13ae51
@jdunkerley jdunkerley force-pushed the wip/jd/excel-append-182309853 branch from 9d165d3 to f13ae51 Compare July 5, 2022 09:39
@jdunkerley jdunkerley marked this pull request as ready for review July 5, 2022 15:54
@jdunkerley jdunkerley requested a review from hubertp July 5, 2022 15:54
radeusgd
radeusgd reviewed Jul 6, 2022
View reviewed changes
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Map_Columns.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Map_Columns.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Io/Excel.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Io/Excel.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Io/Excel.enso Outdated Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/write/ExcelWriter.java Outdated Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/write/ExcelWriter.java Outdated Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/write/ExcelWriter.java Outdated Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/write/ExcelWriter.java Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/write/ExistingDataMode.java Outdated Show resolved Hide resolved
radeusgd
radeusgd requested changes Jul 6, 2022
View reviewed changes
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good overall, but some documentation seems to be suggesting different behavior than the implementation. Unless I'm mistaken, these should be aligned as otherwise the docs will be misleading.

Other than that, some stylistic suggestions.

std-bits/table/src/main/java/org/enso/table/write/ExcelWriter.java Outdated Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/util/ColumnMapper.java Outdated Show resolved Hide resolved
@jdunkerley jdunkerley requested a review from radeusgd July 6, 2022 15:56
radeusgd
radeusgd reviewed Jul 6, 2022
View reviewed changes
test/Table_Tests/src/Excel_Spec.enso Outdated
(Enso_Project.data / test_sheet_name) . copy_to out
extra_another = Table.new [['A', ['d', 'e']], ['B',[4, 5]], ['C',[True, False]], ['D', ['2022-01-20', '2022-01-21']]]
expected = Table.new [['AA', ['a','b','c','d', 'e']], ['BB',[1,2,3,4,5]], ['CC',[True, False, False, True, False]]]
extra_another.write out (File_Format.Excel (Cell_Range "Another!A1")) on_existing_file=Existing_File_Behavior.Append column_mapping=Match_Columns.By_Position
Copy link
Member

Choose a reason for hiding this comment

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

I just realised that because we use the top-left cell A1 as our reference point, we don't really check if we correctly handle the table location - we could completely ignore this coordinate and just append to the first table in the sheet, like the Sheet "foo" selector would, and still pass this test... Which isn't great.

I'd suggest to modify this test to locate the table somewhere else than in the top-left corner.
Also to make things a bit more complicated, I'd suggest placing another table somewhere else within that sheet.

For example like this:
image

radeusgd
radeusgd approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

I like the changes.
I don't want to stall the PR, so let's get this merged, but please let's add the edge test cases, that we've been discussing, in a separate PR.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jul 7, 2022
@mergify mergify bot merged commit 16e6f2f into develop Jul 7, 2022
@mergify mergify bot deleted the wip/jd/excel-append-182309853 branch July 7, 2022 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@4e6 4e6 4e6 approved these changes

@JaroslavTulach JaroslavTulach Awaiting requested review from JaroslavTulach JaroslavTulach is a code owner

@hubertp hubertp Awaiting requested review from hubertp hubertp is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdunkerley @4e6 @radeusgd