feat(collections): add joinToString #1223

majidsajadi · 2021-09-12T13:56:52Z

No description provided.

LionC

Thanks for implementing this! I left some minor comments.

collections/join_to_string.ts

collections/join_to_string_test.ts

majidsajadi · 2021-09-12T18:36:41Z

@LionC updated. let me know if anything else you have in mind.

LionC · 2021-09-12T19:45:07Z

collections/join_to_string.ts

+ * Example:
+ *
+ * ```ts
+ * import { joinToString } from "./join_to_string.ts";


See #1227 , I think we should use the same import as in the README here

oh, nice. that was something that bothered me a lot. I didn't know somebody fixed this in another PR.

kt3k · 2021-09-13T05:53:52Z

@majidsajadi @LionC How about adding limit and truncated options as well from kotlin's version? https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/join-to-string.html

The current proposed API looks a little too trivial to me to be included in std..

majidsajadi · 2021-09-13T05:59:57Z

@majidsajadi @LionC How about adding limit and truncated options as well from kotlin's version? https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/join-to-string.html

The current proposed API looks a little too trivial to me to be included in std..

I guess limit and truncated options are something that would be very useful. the current implemented version can be replaced by a one-liner. array.map(selector).join(separator). adding more options makes the module valuable.

P.S. how about transform? that's something I guess we can use a lot?

kt3k · 2021-09-13T06:07:33Z

@majidsajadi

P.S. how about transform? that's something I guess we can use a lot?

I guess transform is already mapped to selector in our case

LionC · 2021-09-13T12:38:02Z

I am fine with adding limit and truncate. I think in TS, having a subobject might make more sense for safer typing:

type JoinToStringOptions = {
  separator?: string;
  prefix?: string;
  suffix?: string;
  truncate?: {
    after: number;
    with?: string;
  };
};

That will ensure that if you truncate, you need to set a limit and the optional truncate suffix thing (in kotlin you do not really have objet literals, you need to call constructors, so they tend to flatten parameters).

We could also think abot adding a lastSeparator string option that sets a different separator for the very last join, wich allows for the classic A, B, C and D thing for e.g. log messages.

I want to note though @kt3k, that the current version should already be faster than .map().join(), which is a good reason in itself.

majidsajadi · 2021-09-13T15:10:32Z

type JoinToStringOptions = {
  separator?: string;
  prefix?: string;
  suffix?: string;
  truncate?: {
    after: number;
    with?: string;
  };
};

we settle on this? should i update the MR? @kt3k @LionC

kt3k · 2021-09-13T16:12:36Z

we settle on this? should i update the MR? @kt3k @LionC

That design looks good to me. Let's settle on it. It'd be helpful if you could update it

getspooky · 2021-09-16T09:11:28Z

collections/join_to_string.ts

+  array: readonly T[],
+  selector: (el: T) => string,
+  {
+    separator = ", ",


I think space separator is more appropriate

Suggested change

separator = ", ",

separator = " ",

Array.join and Kotlin's joinToString both have the comma in their default, why should we deviate?

Suggested change

separator = ", ",

separator = ",",

Agree with @LionC here, I think it's better if it match Array.prototype.join with a single comma without space.

@zhangyuannie @LionC Array.prototype.join default separator is "," but Kotlin's joinToString default separator is ", ".

I copied my default parameters from Kotlin's joinToString but I guess it's better to go with Array.prototype.join in this case. more familiar API for JS developers.

zhangyuannie

Personally, I'm against this API as it stands right now since

array.map(selector).join(separator)

is more idiomatic and faster.

I agree with @kt3k that we should include limit and truncated for this to be included in std.

I want to note though @kt3k, that the current version should already be faster than .map().join(), which is a good reason in itself.

@LionC Do you have any source for your claim?
With an array of length 500 with { num: Math.random() * 100 } as element and obj => "" + Math.floor(obj.num) as selector, current implementation takes 500ms while map-join takes 150ms.

zhangyuannie · 2021-09-16T17:55:57Z

collections/join_to_string.ts

+  let ret = prefix;
+
+  array.forEach((it, index) => {
+    if (index > 0) {
+      ret += separator;
+    }
+
+    ret += selector(it);
+  });
+
+  ret += suffix;
+
+  return ret;


Suggested change

let ret = prefix;

array.forEach((it, index) => {

if (index > 0) {

ret += separator;

}

ret += selector(it);

});

ret += suffix;

return ret;

return `${prefix ?? ""}${array.map(selector).join(separator)}${suffix ?? ""}}`

User might pass null for prefix and suffix, I don't think they want null literally in the string.

While string concatenation is heavily optimised in v8, I think map is more idiomatic and faster. When we introduce limit and truncated, we should still use an intermediate array instead of doing string concatenation.

User might pass null for prefix and suffix, I don't think they want null literally in the string.

i guess passing null for prefix or suffix throw Type 'null' is not assignable to type 'string'. error.

Only if you use TypeScript with type checking enabled 😉. With the core team's push on removing type check by default for TypeScript, I think it might be worthy to start adding these trivial type guards. Feel free to discard that suggestion through, I don't have too strong of an opinion on this.

I am very strongly against introducing special code to protect JS consumers from passing the wrong types - that is what TS is for.

zhangyuannie · 2021-09-16T17:56:52Z

collections/join_to_string.ts

+  array: readonly T[],
+  selector: (el: T) => string,
+  {
+    separator = ", ",


Suggested change

separator = ", ",

separator = ",",

Agree with @LionC here, I think it's better if it match Array.prototype.join with a single comma without space.

…join-to-string

majidsajadi · 2021-10-09T14:14:58Z

@kt3k review, please.

LionC · 2021-10-10T17:44:34Z

collections/join_to_string.ts

+  limit?: number;
+  truncated?: string;


I am not a fan of that type, as it allows to set truncated without setting limit, which does not make sense. I think this should be

Suggested change

limit?: number;

truncated?: string;

truncate?: {

limit: number;

truncation?: string;

};

or something similar to leverage the compiler catching wrong usage

if we use your suggested type:

export function joinToString<T>( array: readonly T[], selector: (el: T) => string, { separator = ",", prefix = "", suffix = "", truncate = { truncated: "...", limit: -1, } }: Readonly<JoinToStringOptions>, )

{ truncated: "...", limit: -1, }

this is the default value for the truncate option. what if the user overrides this with only limit since the truncated suppose to be optional. the truncated would be undefined.

joinToString(someArray, { truncate: { limit: 3 } });

I know I can handle the default values in the function body but I guess the code would be a little bit messy and I don't know if we want to go that way. the implemented type is just like the Kotlin version of this method so I guess its some what okay.

@LionC

I agree with @majidsajadi . Let's keep the implementation simple. I think it's mostly obvious that truncated is only meaningful with limit set in this particular case.

LionC · 2021-10-10T17:46:32Z

collections/join_to_string.ts

+  let result = "";
+
+  let index = -1;
+  while (++index < array.length) {


Nitpick: I am personally not the biggest fan of implicit mutation with increment operators, as it gets missed quite easily when reading the code, hiding the core step of the loop. I always find it a lot cleaner to just have a separate

index += 1

line

LionC · 2021-10-10T17:47:43Z

collections/join_to_string.ts

+  let result = "";
+
+  let index = -1;
+  while (++index < array.length) {


Why is this not a for loop? Or a do .. while loop? The index initialisation with -1 looks odd to me.

i need to access the index of the element. i cant use forEach since I need to have the option to break the loop.
i know we can use array.entries but I thought it would affect the performance (maybe? I'm not sure.)

I saw this pattern (using while for loop through the arrays) in lodash a lot. so I used that here too. but I'm open to any suggestion that makes the code more clear.

@LionC

LionC · 2021-10-10T17:50:12Z

collections/join_to_string.ts

+  result = prefix + result + suffix;
+
+  return result;


Nitpick: Could be shortened to directly return:

Suggested change

result = prefix + result + suffix;

return result;

return `${prefix}${result}${suffix}`;

CLAassistant · 2021-10-16T04:56:40Z

All committers have signed the CLA.

kt3k

Thank you @majidsajadi for your contribution!

LGTM

kt3k · 2021-10-16T05:01:30Z

@majidsajadi Sorry we changed the CLA a little bit (changed the contact email address), and we need you to sign it again..

majidsajadi · 2021-10-16T18:55:43Z

@kt3k Done.

kt3k · 2021-10-17T11:09:13Z

Thanks!

feat(collections): add joinToString

8e60ef0

LionC suggested changes Sep 12, 2021

View reviewed changes

LionC mentioned this pull request Sep 12, 2021

[Umbrella] collections APIs #1173

Closed

44 tasks

ref(collections): fix lint errors

18f146a

LionC reviewed Sep 12, 2021

View reviewed changes

getspooky suggested changes Sep 16, 2021

View reviewed changes

zhangyuannie reviewed Sep 16, 2021

View reviewed changes

majidsajadi added 3 commits September 17, 2021 20:58

add truncate

1c09899

Merge branch 'main' of https://github.com/denoland/deno_std into add-…

ba24675

…join-to-string

ref: add truncated to joinToString

2d8b39a

LionC suggested changes Oct 10, 2021

View reviewed changes

Merge branch 'main' into add-join-to-string

1ab556e

kt3k approved these changes Oct 16, 2021

View reviewed changes

chore: fmt

aef8e42

kt3k merged commit 2ce4b7e into denoland:main Oct 17, 2021

traceypooh pushed a commit to traceypooh/deno_std that referenced this pull request Nov 14, 2021

feat(collections): add joinToString (denoland#1223)

007264f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(collections): add joinToString #1223

feat(collections): add joinToString #1223

majidsajadi commented Sep 12, 2021

LionC left a comment

majidsajadi commented Sep 12, 2021

LionC Sep 12, 2021

majidsajadi Sep 13, 2021

kt3k commented Sep 13, 2021

majidsajadi commented Sep 13, 2021 •

edited

Loading

kt3k commented Sep 13, 2021

LionC commented Sep 13, 2021

majidsajadi commented Sep 13, 2021

kt3k commented Sep 13, 2021

getspooky Sep 16, 2021

LionC Sep 16, 2021

zhangyuannie Sep 16, 2021

majidsajadi Sep 17, 2021

zhangyuannie left a comment

zhangyuannie Sep 16, 2021

majidsajadi Sep 17, 2021

zhangyuannie Sep 17, 2021

LionC Sep 17, 2021

zhangyuannie Sep 16, 2021

majidsajadi commented Oct 9, 2021

LionC Oct 10, 2021 •

edited

Loading

majidsajadi Oct 16, 2021 •

edited

Loading

kt3k Oct 16, 2021

LionC Oct 10, 2021 •

edited

Loading

LionC Oct 10, 2021

majidsajadi Oct 16, 2021 •

edited

Loading

LionC Oct 10, 2021 •

edited

Loading

CLAassistant commented Oct 16, 2021 •

edited

Loading

kt3k left a comment

kt3k commented Oct 16, 2021

majidsajadi commented Oct 16, 2021

kt3k commented Oct 17, 2021

feat(collections): add joinToString #1223

feat(collections): add joinToString #1223

Conversation

majidsajadi commented Sep 12, 2021

LionC left a comment

Choose a reason for hiding this comment

majidsajadi commented Sep 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kt3k commented Sep 13, 2021

majidsajadi commented Sep 13, 2021 • edited Loading

kt3k commented Sep 13, 2021

LionC commented Sep 13, 2021

majidsajadi commented Sep 13, 2021

kt3k commented Sep 13, 2021

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

zhangyuannie 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

majidsajadi commented Oct 9, 2021

LionC Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

majidsajadi Oct 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LionC Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majidsajadi Oct 16, 2021 • edited Loading

Choose a reason for hiding this comment

LionC Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

CLAassistant commented Oct 16, 2021 • edited Loading

kt3k left a comment

Choose a reason for hiding this comment

kt3k commented Oct 16, 2021

majidsajadi commented Oct 16, 2021

kt3k commented Oct 17, 2021

majidsajadi commented Sep 13, 2021 •

edited

Loading

LionC Oct 10, 2021 •

edited

Loading

majidsajadi Oct 16, 2021 •

edited

Loading

LionC Oct 10, 2021 •

edited

Loading

majidsajadi Oct 16, 2021 •

edited

Loading

LionC Oct 10, 2021 •

edited

Loading

CLAassistant commented Oct 16, 2021 •

edited

Loading