Introduce "StringStream.split()" method #23

f1ames · 2021-10-25T13:21:10Z

This PR introduces:

DataStream.flatMap() method
StringStream class
StringStream.split() method

It also contains some changes regarding BDD test setup (cucumber.js) since the main idea behind it was to provide BDD test for file processing (read -> split -> filter -> aggregate -> reduce) and so it required quite a lot of changes. At the moment, there is still no aggregate and reduce but I will most probably add it in separate PRs since this one got quite big anyways.

DataStream.flatMap() method have been introduced in dc9ad38 (with some minor corrections added later).

As for StringStream and its .split() method:

52c15dd
e033c99
26d803d (it was reworked here)

I also did some refactoring here and there so there are some changes not directly related to the above.

MichalCz

Some change proposals

.yarn/plugins/@yarnpkg/plugin-echo-execute.cjs

MichalCz · 2021-10-25T15:23:08Z

src/streams/base-stream.ts

@@ -9,8 +9,9 @@ import { TransformFunction } from "../ifca";
 // thus the workaround with BaseStream and BaseStreamCreators used below.

 export interface BaseStream<T extends any> {
-    map<U>(callback: TransformFunction<T, U>): BaseStream<U>;
-    filter(callback: TransformFunction<T, Boolean>): BaseStream<T>;
+    map<U, W extends any[]>(callback: TransformFunction<T, U, W>, ...args: W): BaseStream<U>;


A bit of a question here: how will we know that the input is T from the return value?

It should be the same as IFCA where we convert T -> U so it's an IFCA<T,U>

Not sure if I understand what you mean 🤔

We know the callback input (so each chunk) is of type T based on stream generic type (BaseStream<T>). So for methods we need to define output type since input is defined while constructing new stream instance.

Something like:

type F<T,U> = (chunk: T) => U; interface IFCA<T,U> { write(input:T); read(output:U); } class BaseStream<T,U=T> { protected ifca: IFCA<T,U> write(chunk: Parameters<typeof this.ifca.write>[0]); read(chunk: ReturnType<typeof this.ifca.read>); map(mapper: F<U,V>): BaseStream<T,V> } const test = new BaseStream<string>() .map(x => parseInt(x)) .map(x => ({x}) as {x: number}) test.write() test.read()

Ok, we have discussed it F2F and now I understand what's the issue. At the moment:

new BaseStream<string>() .map(x => parseInt(x)) .map(x => ({x}) as {x: number})

returns BaseStream<number> which means it can accept only numbers. The idea is that any transform operation which returns new (or mutated stream) should return a stream which accepts values of the same type as initial stream and yields values of "requested type". In general:

BaseStream.from<InputType, OutputType>() .transform<Type1>() .transform<Type2>() => BaseStream<InputType,Type2>

And the idea is we either mutate this (that's what we agreed on now) or alternatively return new stream instance (but then all intermediate streams, created while chaining, should be somehow made unusable).

I will cover this is a separate PR since it's quite significant change.

@jan-warchol I have extracted it to https://app.clickup.com/t/1j4hr63 (no extensive description for now) but we can discus it if you need any clarifications (I guess it would/should be similar in python after typings are added).

src/streams/base-stream.ts

src/streams/data-stream.ts

src/streams/string-stream.ts

f1ames · 2021-10-27T11:49:57Z

@jan-warchol @MichalCz ready for another review 👍

jan-warchol

🤯
tyle powiem.

test/streams/data/creation.spec.ts

jan-warchol · 2021-10-27T16:40:41Z

src/streams/base-stream.ts

@@ -9,8 +9,9 @@ import { TransformFunction } from "../ifca";
 // thus the workaround with BaseStream and BaseStreamCreators used below.

 export interface BaseStream<T extends any> {
-    map<U>(callback: TransformFunction<T, U>): BaseStream<U>;
-    filter(callback: TransformFunction<T, Boolean>): BaseStream<T>;
+    map<U, W extends any[]>(callback: TransformFunction<T, U, W>, ...args: W): BaseStream<U>;


src/streams/base-stream.ts

src/types.ts

scramjet-bot · 2021-10-28T08:12:20Z

Task linked: CU-1gh7wf4 Czytanie z pliku nie powinno czytać line by line

scramjet-bot · 2021-10-28T08:12:45Z

Task linked: CU-1hyrf4q Przemyśleć generyki

This commit introduces '.flatMap()' method implementation which is consistent with JavaScript 'Array.prototype.flatMap()'. Since 'flatMap' is internally 'map -> filter (empty results) -> flatten' I have reused '.map()' and '.filter()` methods and only added the missing part which is writing flattened chunks to new DataStream instance which is then returned. This is done through async generator which ensures newly created data stream will read new chunks only when it can keeping back pressure intact.

'StringStream.split' needs to be aware when the stream ends to yield chunks which may be not yielded yet. This typically occurs when there is no split sequence present (so all chunks glued together are yielded at once after last chunks is processed).

According to https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored.

This commit corrects the way '.from()' method uses generics by providing 'Constructor' type passed as frist argument. This also results in TS correctly recognize types in most cases so I have removed explicit generics from most of the tests.

Since DataStream is in fact the most basic class and provides generic from there is no need for additional complexity.

f1ames · 2021-11-03T10:17:56Z

Rebased onto latest main.

MichalCz requested changes Oct 25, 2021

View reviewed changes

f1ames requested a review from MichalCz October 27, 2021 07:40

f1ames marked this pull request as ready for review October 27, 2021 07:51

f1ames requested review from jan-warchol and ErykSol October 27, 2021 07:56

Base automatically changed from task/test-scenarios to main October 27, 2021 07:59

jan-warchol reviewed Oct 27, 2021

View reviewed changes

f1ames requested a review from jan-warchol October 28, 2021 15:02

jan-warchol approved these changes Oct 29, 2021

View reviewed changes

f1ames added 18 commits November 1, 2021 02:56

Init cucumber (JS)

47f13ba

Cucumber setup now supports TS

dfac066

Improve yarn/cucumber setup

774b633

Add first 'DataStream.split' test

b633f80

Introduce StringStream (WIP)

e62eff9

Update BaseStream interface

800c54b

Reorganize tests; add StringStream basic tests

3017baa

Handle case with no spli sequence present in '.split()' method

bfa5bad

Add handling of empty split results on chunk end + beginning

f4a766c

Simplify StringStream by removing class generic type

fc2ad5e

Polish tests (WIP)

08d2d04

Rework StringStream.split

5881b7f

'StringStream.split' needs to be aware when the stream ends to yield chunks which may be not yielded yet. This typically occurs when there is no split sequence present (so all chunks glued together are yielded at once after last chunks is processed).

Fix for a case when split sequence is span across multiple chunks

876e050

Fix BDD scenario

a466d65

Drop npm cache

2b6e46b

Fix yarn setup

0f9cc6f

According to https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored.

Remove getter leftover

aee4dba

f1ames added 12 commits November 1, 2021 02:59

Update yarn

62ff8ac

Rework StringStream splitter

0025beb

Fix 'DataStream.from' generic types

d3b2fbc

This commit corrects the way '.from()' method uses generics by providing 'Constructor' type passed as frist argument. This also results in TS correctly recognize types in most cases so I have removed explicit generics from most of the tests.

Read now retruns this for simplicity and to allow chaining

3390bed

Extract common types to separate file

7d0b962

Remove BaseStreamCreators abstract class

66c4ca6

Since DataStream is in fact the most basic class and provides generic from there is no need for additional complexity.

'DataStream.flatMap' can now operate on Iterables too

19c2068

Minor DataStream refactoring

3fdc6db

Minor correction to 'fromFile' creator

1c603a6

Simplify flattening and extract internal logic to reusable method

ca25343

Fix 'BaseStream.flatMap' declaration

873672d

Update 'yarn.lock'

d653507

f1ames force-pushed the feature/stringstream-split branch from 7b167c9 to d653507 Compare November 3, 2021 10:13

f1ames merged commit 6f55422 into main Nov 3, 2021

f1ames deleted the feature/stringstream-split branch November 3, 2021 10:29

f1ames mentioned this pull request Nov 5, 2021

Polymorphic/generic this as return type for subclasses #28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce "StringStream.split()" method #23

Introduce "StringStream.split()" method #23

f1ames commented Oct 25, 2021 •

edited

Loading

MichalCz left a comment

MichalCz Oct 25, 2021

MichalCz Oct 25, 2021

f1ames Oct 26, 2021

MichalCz Oct 27, 2021

f1ames Oct 27, 2021

jan-warchol Oct 27, 2021

f1ames Oct 28, 2021

f1ames commented Oct 27, 2021

jan-warchol left a comment

jan-warchol Oct 27, 2021

scramjet-bot commented Oct 28, 2021

scramjet-bot commented Oct 28, 2021

f1ames commented Nov 3, 2021

Introduce "StringStream.split()" method #23

Introduce "StringStream.split()" method #23

Conversation

f1ames commented Oct 25, 2021 • edited Loading

MichalCz 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

f1ames commented Oct 27, 2021

jan-warchol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scramjet-bot commented Oct 28, 2021

scramjet-bot commented Oct 28, 2021

f1ames commented Nov 3, 2021

f1ames commented Oct 25, 2021 •

edited

Loading