deno_std

deno_std copied to clipboard

Insert columns argument inside options object to make it optional in case stringify is called.

I'm open to implementation ideas and anything else. This PR addresses #882

I could also work at the type safety of the property accessors, but I would need some help. I'm going to send this PR in the deno discord server.

If I run the code like the below, it prints only empty lines

import { stringify } from "./encoding/csv.ts";
const str = await stringify([[1, 2, 3], [4, 5, 6]]);
console.log(str);

This default behavior doesn't seem useful to me.

This is going to be a breaking change, but how much is it difficult to support old behavior as well by function overloading?

If we can overload the function like:

export async function stringify(
  data: DataItem[],
  options: StringifyOptions = {},
): Promise<string>;
export async function stringify(
  data: DataItem[],
  columns: Column[],
  options: StringifyOptions = {},
): Promise<string>;
// ... impl here ...

and keep the old behavior, then it'd be much easier to land this PR

The backwards compatibility could be done, I'll work on that. About the default behavior, I'll try to make something more useful too.

Thanks for the review!

The backwards compatibility could be done, I'll work on that. About the default behavior, I'll try to make something more useful too.

Great! Thanks!

@kt3k I took a look at TypeScript's documentation and it seems that the function overloading cannot be done. Typescript doesn't support function overloading with different number of parameters.

But I'm still working on some useful behavior to make things right.

@kt3k also I think that this default behavior actually makes sense, what could be done is if the user sends a two dimensions array but no accessor at the column option we could throw an error saying that no property acessor was provided in the options column.

If I try to make another default behavior out of this, it could lead to a major confusion with the data, I think that this is one of the problems that could be solved with improving the docs of the function. What do you think?

@luk3skyw4lker

I took a look at TypeScript's documentation and it seems that the function overloading cannot be done. Typescript doesn't support function overloading with different number of parameters.

Overloading with different number of parameters should be possible. We already have such APIs. See assertThrows in testing module for example https://github.com/denoland/deno_std/blob/564d2ed8abe47f97e9198aebefe20b4fea6c3c47/testing/asserts.ts#L599-L619

If I try to make another default behavior out of this, it could lead to a major confusion with the data, I think that this is one of the problems that could be solved with improving the docs of the function. What do you think?

Doc improvement instead of code change is also very welcome. Maybe let's follow that path in this case?

@kt3k Right, maybe I got something wrong, I thought I saw something like this at Deno too, even got confused when checked the docs and they said it wasn't supported.

About the docs, I think I may have some idea to make it work without the property accessors anyway, but it would only work with arrays, which makes more sense to me what do you think? The docs improvement would also be welcoming, the original issue mentions that too.

@kt3k I just did some refactoring and I think that the default behavior for arrays is more useful now, if you test it out with the same case that you pointed out earlier in the PR, the output should be:

1,2,3 4,5,6

I'll redo this PR next weekend if it still makes sense (moving my changes to csv.ts)

@kt3k @bartlomieju @crowlKats please let me know if this changes would still be useful

I would be in favour of packing columns into options instead of overloading. std is not stable and it is versioned. Breaking changes are possible.

I would be in favour of packing columns into options instead of overloading. std is not stable and it is versioned. Breaking changes are possible.

@timreichen That was the first implementation, but I don't think that overloading is much of a problem, it's already done btw. But if it seems like something that wouldn't be useful we can remove it.

@luk3skyw4lker Then I would suggest to implement it but to add a deprecation tag to the old implementation overload, something along the lines of

/**
  * @deprecated this overload will be removed soon. Use `stringify(data, { ...options, columns })` instead.
  */

I think the columns as a property in options is cleaner and both overloads provide the same data.

@timreichen Alright! I'll update this PR next weekend with the suggestions, thanks for the tips!

@timreichen @kt3k I have just noticed that for this to land, it would be a breaking change anyway, because for it to be available even with the overloading, I have to change the order of the function parameters, so if it`s gonna be a breaking change anyway, I think that we don't need the overloading. What do you guys think?

@luk3skyw4lker

it would be a breaking change anyway, because for it to be available even with the overloading, I have to change the order of the function parameters, so if it`s gonna be a breaking change anyway, I think that we don't need the overloading.

Ok. Let's try without overloading

@kt3k @timreichen could you guys take a look? I'm open to any suggestions on how to make it better, thanks for the patience, previous suggestions and reviews!

@timreichen alterations done!

LGTM!

We also introduced a few breaking changes to encoding/csv recently, but we didn't receive much response from the community. https://github.com/denoland/deno_std/pull/2536 https://github.com/denoland/deno_std/pull/2491

I think it's still relatively safe to make breaking changes to encoding/csv.