-
-
Notifications
You must be signed in to change notification settings - Fork 150
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.
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
[transducers] Transducers crash when source is an empty string #186
Comments
Technically that assumes ES2015. In ES5 an array is truthy but doesn't have a function at Indeed, a truly un-shimmed ES5 environment would crash just by calling I don't know whether people are using |
Hi @gavinpc-mindgrub - thank you for this! Strings (incl. empty ones) are indeed supposed to be fully supported as iterables and so the more proper check seems unavoidable. I'm not worried about ES5 compatibility since this is explicitly stated in the main readme that all packages are distributed as ES6 w/o any polyfills... Btw. The alternatives I was originally considering to completely avoid the function overloads and checks for a final iterable arg was to provide additional functions like |
@gavinpc-mindgrub do you have bandwidth for a PR? No worries if not, just let me know pls... |
@postspectacular, sure I'll send something this evening (I'm EST). Should this branch off of master? On the design question, I have found the overloads convenient for sketching and "normal" usage. I am mostly working on metaprogramming, though, and in that context I have made my own wrappers anyway (to support lazy, serializable bindings representing a transducer description). |
@gavinpc-mindgrub great & thank you. There's no rush & please branch off I'd also say to stick with the overloads to avoid breakage of existing code and yet another major version change. (Btw. Looking v.forward to see what you've been up to with this all... :) ) |
@postspectacular, roger that. I currently get the following test failures when running off of develop. Before I dig into these, are they currently expected? The last one may be a legit timeout. I am working right now on a PC that is stupid slow.
|
I've been getting them sporadically on travis (never locally) and assuming these are just issues with the timing related test setup and for whatever reason slow or super imprecise native timers in node. am 99.999% sure you can ignore these and/or rerun... also see travis build history |
@postspectacular I have this more or less done. Question about export function flatten<T>(src?: Iterable<T | Iterable<T>>): any {
return flattenWith(
(x: any) =>
x != null && x[Symbol.iterator] && typeof x !== "string"
? <any>x
: undefined,
src!
);
} So right now, the following tests pass: assert.deepEqual([...flatten(["", "a"])], ["", "a"]);
assert.deepEqual([...flatten([[], ["a"], ""])], ["a", ""]);
// (I added these)
assert.deepEqual([...flatten([["abc"]])], ["abc"]);
assert.deepEqual([...flatten(["abc"])], ["abc"]);
assert.deepEqual([...flatten("abc")], ["a", "b", "c"]);
assert.deepEqual([...flatten([""])], [""]);
If this is correct: > [...tx.flatten("abc")]
[ 'a', 'b', 'c' ] Then I don't want to add a special case just for |
Thank you for the in-depth analysis, @gavinpc-mindgrub! In this case I'd opt (like I guess you do too) for consistency, even if it means breaking the current behavior for top-level strings (incl. empty ones). So I'd say we treat top-level strings as atoms (unflattenable, just like nested strings) and update the body of return isIterable(src)
? iterator(flattenWith(fn), isString(src) ? <any>[src] : src)
: (rfn: Reducer<any, T>) => { ... } With that change we get:
|
@postspectacular I haven't forgotten about this, just a bit caught up in holiday/family at the moment. Holdup was mostly learning my way around to include proper tests. I did come across a question, though. I wasn't quite sure exactly how
Thanks, & best wishes for 2020! |
Hi @gavinpc-mindgrub - happy new year to you!! 🎉 I thought we cleared up how the behavior of Also I'd say don't bother too much w/ docs for this PR, they can always be added/edited later on. Thank you for doing this all! 👍 |
Hi @gavinpc-mindgrub - I was going to prepare a new release in the next few days and was wondering if you have an ETA of your transducer updates and/or if you still have the time to work on this at all... no shame in saying no or needing more time, I just would like to synchronize, if poss. Else we can also just do another one once you're ready... Thanks! |
@postspectacular gotcha. I'll shoot for this evening, but if it doesn't come through then we can kick to the next round. |
@gavinpc-mindgrub no pressure pls! take your time! |
…urce for several transducers.
fix(transducers): #186, Fix crash when using empty string as source for several transducers
* 'develop' of github.com:thi-ng/umbrella: fix(transducers): #186, Fix crash when using empty string as source for several transducers.
* develop: Publish docs(transducers): update readme refactor(transducers): update flatten & flattenWith test(transducers): minor update flatten tests fix(transducers): #186, Fix crash when using empty string as source for several transducers.
Most transducers take a source sequence as a final argument. Since strings are iterable, so you can do this:
However, if you provide an empty string, they will crash:
This is obviously because the transducer functions do a truthiness test on their final argument to determine whether a source was provided:
This could be corrected by replacing
src
withisIterable(src)
(using@thi.ng/checks
). This would incur the additional work of looking upSymbol.iterator
, but it would seem to be more correct. I can't think of a case where this would be a breaking change (i.e. values forsrc
that work now but are not iterable by that test).@postspectacular I'm willing to PR if you think this is actionable. I understand there's a lot of branch work going on, so whatever you think best.
A workaround for the time being is to wrap the string argument in
[...s]
.The text was updated successfully, but these errors were encountered: