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

Use data type map instead of overloads #1317

Merged
merged 21 commits into from
Feb 22, 2024
Merged

Conversation

Planeshifter
Copy link
Member

Description

What is the purpose of this pull request?

This pull request:

  • proposes changing the TypeScript definitions for packages accepting a dtype parameter and returning an array of the respective type to not use function overloads but instead a type map. This allows for cutting down on the resulting bloat and will make the codebase more maintainable, as adding support for a new data type would simply require updating the type map inside @stdlib/types.

One potential concern is Intellisense showing opaque type information, but I argue that this is a non-issue if we have appropriate TSDoc comments, as is the case in this example where we enumerate the possible values in the comment.

Screenshot 2024-02-16 at 2 29 28 PM

Furthermore, the current overloading strategy also is not ideal from an Intellisense point of view, since it forces users to manually step through the available overloads to see what inputs are allowed:

image

(there is a 1/12 stepper button in the bottom left to toggle between the overloads; covered by the Copilot interface for some reason but usually it's fully visible...)

Related Issues

Does this pull request have any related issues?

No.

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@kgryte kgryte added the TypeScript Issue involves or relates to TypeScript. label Feb 17, 2024
@Planeshifter Planeshifter marked this pull request as ready for review February 17, 2024 20:51
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@Planeshifter All in all, this is looking good. And the changes certainly condense declarations and better preserve type information. In general, I think we need to rename the type maps and do so using an existing naming convention. Personally, I don't have an issue with verbosity, given auto-complete and aliasing.

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Copy link
Member Author

@kgryte Feedback has been addressed, PR is now ready for another look!

Signed-off-by: Athan <kgryte@gmail.com>
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @Planeshifter. Left a few comments concerning when to use DataTypeMap vs when to use NumericAndGenericDataTypeMap.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @Planeshifter! Let's get this in. Any further updates can be addressed in follow-up PRs.

@kgryte kgryte merged commit 819d2e4 into develop Feb 22, 2024
7 checks passed
@kgryte kgryte deleted the philipp/use-data-type-map branch February 22, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TypeScript Issue involves or relates to TypeScript.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants