Add Columns_To_Add type to move away from Integer | Nothing #10152

marthasharkey · 2024-06-03T08:46:14Z

Pull Request Description

Adds new type "Coulmns_To_Add" that replaces the Integer | Nothing type
And adds new widget to split_to_columns and tokenise_to_columns

Closes #10041

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

distribution/lib/Standard/Database/0.0.0-dev/src/DB_Table.enso

radeusgd · 2024-06-03T14:11:13Z

distribution/lib/Standard/Table/0.0.0-dev/src/Columns_To_Add.enso

+
+## PRIVATE
+type Columns_To_Add
+    ## Read all rows.


This doc comment seems off.

distribution/lib/Standard/Table/0.0.0-dev/src/Columns_To_Add.enso

radeusgd

Looks good, just some small nits

Co-authored-by: Radosław Waśko <[email protected]>

GregoryTravis · 2024-06-03T16:19:17Z

distribution/lib/Standard/Database/0.0.0-dev/src/DB_Table.enso

-        _ = [column, delimiter, column_count, on_problems]
+    @column_count Columns_To_Add.default_widget
+    split_to_columns : Text | Integer -> Text -> Columns_To_Add -> Problem_Behavior -> DB_Table
+    split_to_columns self column delimiter="," (column_count : Columns_To_Add = ..All_Columns) on_problems=Report_Warning =


Please update the Arguments: documentation above for column_count, here, and in the other related spots.

GregoryTravis · 2024-06-03T16:20:43Z

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Fan_Out.enso

@@ -22,11 +23,11 @@ polyglot java import org.enso.table.data.mask.OrderMask
   - input_column: The column to transform.
   - function: A function that transforms a single element of `input_column`
     to multiple values.
-fan_out_to_columns : Table -> Text | Integer -> (Any -> Vector Any) -> Integer | Nothing -> (Integer -> Any) -> Problem_Behavior -> Table | Nothing
-fan_out_to_columns table input_column_id function column_count=Nothing column_builder=make_string_builder on_problems=Report_Error =
+fan_out_to_columns : Table -> Text | Integer -> (Any -> Vector Any) -> Columns_To_Add -> (Integer -> Any) -> Problem_Behavior -> Table | Nothing


Please add argument documentation for column_count.

Co-authored-by: Radosław Waśko <[email protected]>

AdRiley · 2024-06-03T16:46:22Z

test/Table_Tests/src/In_Memory/Split_Tokenize_Spec.enso

@@ -204,7 +205,7 @@ add_specs suite_builder =
            t = Table.new cols
            expected_rows = [[0, "a", "c", Nothing, Nothing], [1, "c", "d", "ef", Nothing], [2, "gh", "ij", "u", Nothing]]
            expected = Table.from_rows ["foo", "bar 1", "bar 2", "bar 3", "bar 3"] expected_rows
-            t2 = t.split_to_columns "bar" "b" column_count=4
+            t2 = t.split_to_columns "bar" "b" column_count=(Columns_To_Add.First 4)


Will the old behaviour still work? I think that it should and we should leave the test in place to ensure that it does

Old behaviour will still work as we have conversion from Integer to Columns_To_Add.
We should keep a test proving this.

AdRiley

Looks good. I just have a question of whether this should be a breaking API change for existing workflows?

jdunkerley

A few little tweaks but looks good.

jdunkerley · 2024-06-03T14:56:37Z

distribution/lib/Standard/Database/0.0.0-dev/src/DB_Table.enso

@@ -39,6 +39,7 @@ import Standard.Table.Internal.Widget_Helpers
 import Standard.Table.Match_Columns as Match_Columns_Helpers
 import Standard.Table.Row.Row
 import Standard.Table.Rows_To_Read.Rows_To_Read
+import Standard.Table.Columns_To_Add.Columns_To_Add


small nit but please put in alphabetical order

distribution/lib/Standard/Table/0.0.0-dev/src/Columns_To_Add.enso

jdunkerley · 2024-06-03T17:06:05Z

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Fan_Out.enso

@@ -7,6 +7,7 @@ import project.Conversions.Convertible_To_Rows.Key_Value
 import project.Internal.Problem_Builder.Problem_Builder
 import project.Table.Table
 import project.Value_Type.Value_Type
+import project.Columns_To_Add.Columns_To_Add


as above please sort alphabetically.

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Split_Tokenize.enso

test/Table_Tests/src/In_Memory/Fan_Out_Spec.enso

jdunkerley · 2024-06-03T17:14:14Z

Looks good. I just have a question of whether this should be a breaking API change for existing workflows?

Don't believe we have a breaking API change. Happy path is unchanged, though unhappy path now a warning not an error but think this is ok.

AdRiley · 2024-06-03T18:09:44Z

Looks good. I just have a question of whether this should be a breaking API change for existing workflows?

Don't believe we have a breaking API change. Happy path is unchanged, though unhappy path now a warning not an error but think this is ok.

My question is can you pass Nothing and an Integer into this parameter as you could before? And should you be able to?

jdunkerley · 2024-06-03T20:18:30Z

Looks good. I just have a question of whether this should be a breaking API change for existing workflows?

Don't believe we have a breaking API change. Happy path is unchanged, though unhappy path now a warning not an error but think this is ok.

My question is can you pass Nothing and an Integer into this parameter as you could before? And should you be able to?

Yes you can (as the auto conversion will be fired).

This is the same as when I change read to take an Rows_To_Read. It still allows backwards compatibility and plays nicely with the widgets. Plus I think the more text friendly .split_to_columns "input" column_count=3 is still a useful shorthand.

Co-authored-by: James Dunkerley <[email protected]>

…okenize.enso Co-authored-by: James Dunkerley <[email protected]>

test/Table_Tests/src/In_Memory/Fan_Out_Spec.enso

marthasharkey added 4 commits May 31, 2024 13:54

basic changes to split_to_columns

4e2e042

split_columns working with widget

f95b2e3

rename type

95858dd

Fan_Out function to take columns_to_add

21bd80c

marthasharkey added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 3, 2024

marthasharkey added 7 commits June 3, 2024 10:24

remove unneeded error

12ce9cb

add columns_to_add to tokenise_to_columns

995b531

update tests

6618b83

set column count to start at 1

324a4ad

add backwards compatibility

eb64128

update tests

b0fa4ab

add import to test file

d8625d8

marthasharkey changed the title ~~Wip/mk/add column count type~~ Add Columns_To_Add type to move away from Integer | Nothing Jun 3, 2024

marthasharkey marked this pull request as ready for review June 3, 2024 13:29

marthasharkey requested review from jdunkerley, radeusgd, GregoryTravis and AdRiley as code owners June 3, 2024 13:29

radeusgd reviewed Jun 3, 2024

View reviewed changes

distribution/lib/Standard/Database/0.0.0-dev/src/DB_Table.enso Outdated Show resolved Hide resolved

radeusgd reviewed Jun 3, 2024

View reviewed changes

distribution/lib/Standard/Table/0.0.0-dev/src/Columns_To_Add.enso Outdated Show resolved Hide resolved

radeusgd approved these changes Jun 3, 2024

View reviewed changes

Update distribution/lib/Standard/Table/0.0.0-dev/src/Columns_To_Add.enso

34ff6b7

Co-authored-by: Radosław Waśko <[email protected]>

GregoryTravis approved these changes Jun 3, 2024

View reviewed changes

marthasharkey and others added 2 commits June 3, 2024 17:25

Update distribution/lib/Standard/Database/0.0.0-dev/src/DB_Table.enso

bbf2b41

Co-authored-by: Radosław Waśko <[email protected]>

update doc

2f0f4df

AdRiley reviewed Jun 3, 2024

View reviewed changes

AdRiley requested changes Jun 3, 2024

View reviewed changes

jdunkerley approved these changes Jun 3, 2024

View reviewed changes

marthasharkey and others added 4 commits June 4, 2024 11:23

Update distribution/lib/Standard/Table/0.0.0-dev/src/Columns_To_Add.enso

097eb59

Co-authored-by: James Dunkerley <[email protected]>

Update test/Table_Tests/src/In_Memory/Fan_Out_Spec.enso

2872f13

Co-authored-by: James Dunkerley <[email protected]>

Update distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Split_T…

210b7e0

…okenize.enso Co-authored-by: James Dunkerley <[email protected]>

sort imports alphabetically

8a5d026

AdRiley reviewed Jun 4, 2024

View reviewed changes

test/Table_Tests/src/In_Memory/Fan_Out_Spec.enso Show resolved Hide resolved

keep old tests for backward compatibility

9886629

AdRiley approved these changes Jun 4, 2024

View reviewed changes

jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jun 4, 2024

mergify bot merged commit 24d209a into develop Jun 4, 2024
37 checks passed

mergify bot deleted the wip/mk/add-column-count-type branch June 4, 2024 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Columns_To_Add type to move away from Integer | Nothing #10152

Add Columns_To_Add type to move away from Integer | Nothing #10152

marthasharkey commented Jun 3, 2024 •

edited

Loading

radeusgd Jun 3, 2024

radeusgd left a comment

GregoryTravis Jun 3, 2024

GregoryTravis Jun 3, 2024

AdRiley Jun 3, 2024

jdunkerley Jun 3, 2024

AdRiley left a comment

jdunkerley left a comment

jdunkerley Jun 3, 2024

jdunkerley Jun 3, 2024

jdunkerley commented Jun 3, 2024

AdRiley commented Jun 3, 2024

jdunkerley commented Jun 3, 2024

Add Columns_To_Add type to move away from Integer | Nothing #10152

Add Columns_To_Add type to move away from Integer | Nothing #10152

Conversation

marthasharkey commented Jun 3, 2024 • edited Loading

Pull Request Description

Checklist

radeusgd Jun 3, 2024

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

GregoryTravis Jun 3, 2024

Choose a reason for hiding this comment

GregoryTravis Jun 3, 2024

Choose a reason for hiding this comment

AdRiley Jun 3, 2024

Choose a reason for hiding this comment

jdunkerley Jun 3, 2024

Choose a reason for hiding this comment

AdRiley left a comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

jdunkerley Jun 3, 2024

Choose a reason for hiding this comment

jdunkerley Jun 3, 2024

Choose a reason for hiding this comment

jdunkerley commented Jun 3, 2024

AdRiley commented Jun 3, 2024

jdunkerley commented Jun 3, 2024

marthasharkey commented Jun 3, 2024 •

edited

Loading