[UAC-12] The current blocktype CSVFileExtractor is refactored to an CSVInterpreter. #168
Conversation
Merge remote-tracking branch 'origin/main' into feat-mobility-ext-uac-12-csv-interpreter
| { | ||
| code: blockExample, | ||
| description: | ||
| 'Interprets an input file coming from an GTFS-file-collection as a csv-file containing string-values delimited by `delimiter` and outputs `Sheet`.', |
There was a problem hiding this comment.
This would be better without the GTFS context and with a delimiter attribute, aka just have a block that interprets a file as a CSV file with a delimiter of ;.
| export function parseAsCsv( | ||
| rawData: string, | ||
| delimiter: string, | ||
| blockExecutor: BlockExecutor, |
There was a problem hiding this comment.
This is not good, the right way to split this is have the function return a promise with result or err and an error message and keep the logging and diagnostic parts in the blockexecutor itself.
There was a problem hiding this comment.
Ah okay, that makes sense. Thanks for the input, feedback like that improves my coding skills :D
There was a problem hiding this comment.
changed, not sure, if mine is the nicest solution to handle this error, came upon my typescript limits :D
If you have a better approach, let me know.
| ); | ||
|
|
||
| const sheet = await parseAsCsv(csvFile, delimiter); | ||
| if (sheet instanceof Error) { |
There was a problem hiding this comment.
@rhazn Since my function parseAsCsv returns `Promise<R.Result | Error> if have to check like that, if an error occured. I was not able to use R.err() in parseAsCsv, because as far as i am correct R.err() expects an object containing ast-nodes etc?
There was a problem hiding this comment.
I don't see a reason to keep the Result/Error return, you are refactoring out common code for CSV parsing, the sheet and error handling can (and should) be kept in the executors. I'd probably use Either<string[][] | string>(with the string being an error message) (see how that is set up here https://github.com/jvalue/jayvee/blob/main/libs/execution/src/lib/execution-result.ts#L15) as a return value for your parseAsCSV function and then move the building of the sheet and error handling back into the blocks.
There was a problem hiding this comment.
Either was new for me, thats why i was confused, how to use it. Does it now makes sense?
libs/extensions/tabular/lang/src/lib/csv-interpreter-meta-inf.ts
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| const sheet = await parseAsCsv(csvFile, delimiter); | ||
| if (sheet instanceof Error) { |
There was a problem hiding this comment.
I don't see a reason to keep the Result/Error return, you are refactoring out common code for CSV parsing, the sheet and error handling can (and should) be kept in the executors. I'd probably use Either<string[][] | string>(with the string being an error message) (see how that is set up here https://github.com/jvalue/jayvee/blob/main/libs/execution/src/lib/execution-result.ts#L15) as a return value for your parseAsCSV function and then move the building of the sheet and error handling back into the blocks.
| import * as R from '@jayvee/execution'; | ||
| import { Sheet } from '@jayvee/language-server'; | ||
| import { Either } from 'fp-ts/lib/Either'; | ||
| import * as E from 'fp-ts/lib/Either'; |
There was a problem hiding this comment.
I don't think you need the * import here. Otherwise looks good to me.
Refactores the CSVFileExtractor to an CSVInterpreter. In the downstream UACs of that one, the functionalty of CSVFileExtractor is splitted up to the new CSVInterpreter and HTTPExtractor. Most of the parsing-methods are copied from the CSVFileExtractor.
Part of RFC0002 mobility-extension.
Implements following UACs of #123: