Skip to content
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

refactor(encoding): making column argument optional #2168

Merged
merged 8 commits into from
Sep 7, 2022

Conversation

luk3skyw4lker
Copy link
Contributor

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

Insert columns argument inside options object to make it optional in case stringify is called
@luk3skyw4lker
Copy link
Contributor Author

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.

@kt3k
Copy link
Member

kt3k commented May 4, 2022

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.

@kt3k
Copy link
Member

kt3k commented May 4, 2022

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

@luk3skyw4lker
Copy link
Contributor Author

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!

@kt3k
Copy link
Member

kt3k commented May 4, 2022

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!

@luk3skyw4lker
Copy link
Contributor Author

@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.

@luk3skyw4lker
Copy link
Contributor Author

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

@luk3skyw4lker
Copy link
Contributor Author

@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?

@kt3k
Copy link
Member

kt3k commented May 30, 2022

@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?

@luk3skyw4lker
Copy link
Contributor Author

@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.

@luk3skyw4lker
Copy link
Contributor Author

@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

@luk3skyw4lker
Copy link
Contributor Author

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

@timreichen
Copy link
Contributor

timreichen commented Aug 21, 2022

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.

@luk3skyw4lker
Copy link
Contributor Author

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.

@timreichen
Copy link
Contributor

@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.

@luk3skyw4lker
Copy link
Contributor Author

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

@luk3skyw4lker
Copy link
Contributor Author

@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?

@kt3k
Copy link
Member

kt3k commented Aug 29, 2022

@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

@luk3skyw4lker
Copy link
Contributor Author

@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!

encoding/csv.ts Outdated Show resolved Hide resolved
@luk3skyw4lker luk3skyw4lker requested review from timreichen and removed request for kt3k and bartlomieju September 3, 2022 18:49
@luk3skyw4lker
Copy link
Contributor Author

@timreichen alterations done!

@timreichen
Copy link
Contributor

LGTM!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kt3k
Copy link
Member

kt3k commented Sep 7, 2022

We also introduced a few breaking changes to encoding/csv recently, but we didn't receive much response from the community.
#2536
#2491

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

@kt3k kt3k merged commit d3b14b2 into denoland:main Sep 7, 2022
@luk3skyw4lker luk3skyw4lker deleted the refactor/column_argument_optional branch September 7, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants