Feature/396 handle xlsx files #421

A-M-A-X · 2023-08-10T13:46:40Z

Implemented xlsx interpreter and sheet-picker blocks.
Test cases if applicable will be in a different PR.

…. missing. Lint still failing due to cycle dependendies + type any warning.

fixed missing sheets on xlsx import of wb

A-M-A-X · 2023-08-11T07:33:20Z

Please let me know which classes I should write test cases for and to which extend or if this is not required at this stage. @rhazn

georg-schwarz

Nice work!

I know I wasn't tagged but still wanted to leave you a few comments ;-)

georg-schwarz · 2023-08-11T08:27:47Z

libs/execution/src/lib/types/io-types/workbook.ts

+  addSheet(sheet: Sheet) {
+    this.sheets.push(sheet);
+  }
+
+  addNewSheet(data: string[][], sheetName?: string) {
+    const sheetNameOrDefault = sheetName ?? `Sheet${this.sheets.length + 1}`;
+    this.addSheet(new Sheet(data, sheetNameOrDefault));
+  }


This implementation currently allows sheets with duplicate names. I'd try to avoid this, or is this intended behavior?

Added check to prevent duplicate names in workbook

georg-schwarz · 2023-08-11T08:37:31Z

libs/extensions/tabular/exec/src/lib/sheet-picker-executor.ts

+      PrimitiveValuetypes.Text,
+    );
+    const sheet = workbook.getSheetByName(sheetName);
+    assert(sheet instanceof Sheet);


Rather sheet !== undefined (would be more expressive of your intention imo

removed assert completly as already checked within function getSheetByName().

georg-schwarz · 2023-08-11T10:52:38Z

libs/extensions/tabular/exec/src/lib/xlsx-interpreter-executor.ts

+      const workSheet = workBookFromFile.Sheets[workSheetName];
+      assert(workSheet !== undefined);
+
+      const workSheetDataArray = Array.prototype.map.call<


That line looks weird to me, why go over the prototype?

To be quite frank, because I did't manage to do it differently eg calling map directly without failing lint and or type checks. Maybe I can have another look with @rhazn.

Yes, please bring this up in our sync, we can do it live.

georg-schwarz · 2023-08-11T10:53:16Z

libs/extensions/tabular/exec/src/lib/xlsx-interpreter-executor.ts

+          ) => string[],
+        ],
+        string[][]
+      >(workSheet, (x: xlsx.CellObject[]): string[] => {


pls use speaking variable names to indicate to readers whats happening.

renamed to row

georg-schwarz · 2023-08-11T10:53:40Z

libs/extensions/tabular/exec/src/lib/xlsx-interpreter-executor.ts

+        ],
+        string[][]
+      >(workSheet, (x: xlsx.CellObject[]): string[] => {
+        return x.map<string>((y: xlsx.CellObject) => {


same here: better naming of variables please (e.g., cell)

renamed to cell

georg-schwarz · 2023-08-15T16:41:53Z

Uhm, weird. Didn't mean to close this PR!

rhazn

Left some comments.

Regarding the PR itself: Please make sure to always at least mention the related task, otherwise it is very hard to navigate. You can do so by just adding a 'closes #' or 'relates to #' in the description where number is the issue id.

Regarding test cases, hopefully @georg-schwarz can help me out here, do you know from the top of your head where we are right now with tests, should we add tests for the new blocks or the iotype?

rhazn · 2023-08-16T09:41:00Z

libs/execution/src/lib/debugging/debug-log-visitor.ts

@@ -134,6 +135,12 @@ export class DebugLogVisitor implements IoTypeVisitor<void> {

    this.log('... (omitted in peek mode)');
  }
+  visitWorkbook(workbook: Workbook): void {


Should

have the same empty return with minimal debug granularity as the visitTable function has

Log the number of worksheets in the workbook by default

use the log function consistently, never console.log

log the peek comment (as visit table does)

Added

empty return for minimal debug granularity

Logging the number of worksheets in the workbook

consistently using log function

added peek functionality for bigger workbooks

rhazn · 2023-08-16T09:43:06Z

libs/execution/src/lib/types/io-types/workbook.ts

+
+export class Workbook implements IOTypeImplementation<IOType.WORKBOOK> {
+  public readonly ioType = IOType.WORKBOOK;
+  private sheets: Sheet[];


This can be assigned immediately and you do not need a constructor.

Changed accordingly.

rhazn · 2023-08-16T09:43:24Z

libs/execution/src/lib/types/io-types/workbook.ts

+    this.sheets = [];
+  }
+
+  getSheets(): ReadonlyArray<Sheet> {


Why readonly array? I'd just reuse the Sheet[].

changed accordingly.

rhazn · 2023-08-16T09:47:18Z

libs/execution/src/lib/types/io-types/workbook.ts

+    return this.sheets;
+  }
+
+  getSheetByName(sheetName: string): Sheet {


We shouldn't crash the interpreter with an assert if the sheet does not exist, I'd return Sheet or undefined and use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

rhazn · 2023-08-16T09:49:39Z

libs/execution/src/lib/types/io-types/workbook.ts

+    return sheet;
+  }
+
+  addSheet(sheet: Sheet) {


Why addSheet and addNewSheet with different semantic? addSheet lets you add sheets with duplicate names, addNewSheet does not. This has to be one function, just remove the current addSheet and rename the addNewSheet to addSheet after.

Also, please add return types. These functions should return the workbook itself to enable fluent interfaces (https://en.wikipedia.org/wiki/Fluent_interface).

changed as suggested

rhazn · 2023-08-16T09:55:18Z

libs/execution/src/lib/types/io-types/workbook.ts

For the general approach I am not convinced a name should be the property of a sheet. You run into multiple problems using that approach:

You have complex code because you need to filter

You have the problem of duplicate names and what to do with them

Why not make the name to sheet mapping a property of a workbook? Keep sheets as they are, nameless. Instead, track sheets in the workbook as objects containing the name and the actual sheet. Then you can have an index (from the position of the object in the array), the sheet and the name.

Added it as a map<string, workbook> where string is the name of the workbook.
As discussed with the downside not beeing able to reference the sheet by position within the workbook e.g. read from sheet 5.

rhazn · 2023-08-16T09:56:56Z

libs/extensions/tabular/lang/src/lib/xlsx-interpreter-meta-inf.ts

+    );
+
+    this.docs.description =
+      "Interprets an input file as a xlsx-file and outputs a `Workbook` containing `Sheet`'s.";


Suggested change

"Interprets an input file as a xlsx-file and outputs a `Workbook` containing `Sheet`'s.";

"Interprets an input file as a XLSX-file and outputs a `Workbook` containing `Sheet`s.";

Changed as suggested

rhazn · 2023-08-16T09:57:07Z

libs/extensions/tabular/lang/src/lib/xlsx-interpreter-meta-inf.ts

+      {
+        code: blockExample,
+        description:
+          "Interprets an input file as a xlsx-file and outputs a `Workbook` containing `Sheet`'s.",


Suggested change

"Interprets an input file as a xlsx-file and outputs a `Workbook` containing `Sheet`'s.",

"Interprets an input file as a XLSX-file and outputs a `Workbook` containing `Sheet`s.",

Changed as suggested

rhazn · 2023-08-16T09:59:11Z

libs/extensions/tabular/exec/src/lib/xlsx-interpreter-executor.ts

+    file: BinaryFile,
+    context: ExecutionContext,
+  ): Promise<R.Result<Workbook>> {
+    context.logger.logDebug(`reading from xlsx file`);


Suggested change

context.logger.logDebug(`reading from xlsx file`);

context.logger.logDebug(`Reading from XLSX file`);

rhazn · 2023-08-16T10:01:15Z

libs/extensions/tabular/exec/src/lib/xlsx-interpreter-executor.ts

+      const workSheet = workBookFromFile.Sheets[workSheetName];
+      assert(workSheet !== undefined);
+
+      const workSheetDataArray = Array.prototype.map.call<


Yes, please bring this up in our sync, we can do it live.

georg-schwarz · 2023-08-28T09:14:56Z

@A-M-A-X seems like the dev branch got deleted by mistake, leading to an automation to close this PR. Can you retarget on main and reopen?

A-M-A-X · 2023-08-28T13:16:25Z

@georg-schwarz not sure if I am able to. I guess I am missing rights to do so.

rhazn · 2023-08-28T16:06:30Z

You can create a new PR from your branch to main and keep this one closed.

A-M-A-X · 2023-08-29T09:40:49Z

opened new PR #433

A-M-A-X added 6 commits August 2, 2023 18:04

initial commit. Test model ran successful. Propper error handling etc…

5898db4

…. missing. Lint still failing due to cycle dependendies + type any warning.

fixed lint warnings/errors

a74a470

fixed missing sheets on xlsx import of wb

minor changes to xlsx and sheet meta file

bc9c93b

Merge branch 'dev' into feature/396-Handle-XLSX-files

6b7dcb2

minor change

4d82690

fixed lint error

fd5c19f

A-M-A-X changed the title ~~WIP: Feature/396 handle xlsx files~~ Feature/396 handle xlsx files Aug 10, 2023

rhazn self-requested a review August 10, 2023 16:37

georg-schwarz reviewed Aug 11, 2023

View reviewed changes

A-M-A-X added 3 commits August 14, 2023 13:42

added first feedback from PR

0abeb51

.

f1f0a5c

formatted code

a7898a2

georg-schwarz deleted the branch jvalue:dev August 15, 2023 14:27

georg-schwarz closed this Aug 15, 2023

georg-schwarz reopened this Aug 15, 2023

rhazn requested changes Aug 16, 2023

View reviewed changes

f3l1x98 deleted the branch jvalue:dev August 28, 2023 08:58

f3l1x98 closed this Aug 28, 2023

A-M-A-X mentioned this pull request Aug 29, 2023

Feature/396 handle xlsx files #433

Merged

	"Interprets an input file as a xlsx-file and outputs a `Workbook` containing `Sheet`'s.";
	"Interprets an input file as a XLSX-file and outputs a `Workbook` containing `Sheet`s.";

	context.logger.logDebug(`reading from xlsx file`);
	context.logger.logDebug(`Reading from XLSX file`);

Feature/396 handle xlsx files #421

Feature/396 handle xlsx files #421

Conversation

A-M-A-X commented Aug 10, 2023

A-M-A-X commented Aug 11, 2023

georg-schwarz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georg-schwarz commented Aug 15, 2023

rhazn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georg-schwarz commented Aug 28, 2023

A-M-A-X commented Aug 28, 2023

rhazn commented Aug 28, 2023

A-M-A-X commented Aug 29, 2023