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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding support for intersection of arrays and tuples #1237

Merged
merged 4 commits into from May 6, 2022

Conversation

filipomar
Copy link
Contributor

@filipomar filipomar commented May 1, 2022

Adding support for intersection of arrays and tupples such as:

type NonEmptyArray<T> = [T, ...T[]] & T[];

Types originally from here.

Version

Published prerelease version: v1.1.0-next.3

Changelog

馃帀 This release contains work from new contributors! 馃帀

Thanks for all your work!

鉂わ笍 null@filipomar

鉂わ笍 Remi Cattiau (@loopingz)

鉂わ笍 Hadrien Milano (@hmil)

馃殌 Enhancement

馃悰 Bug Fix

馃敥 Dependency Updates

Authors: 8

return this.childTypeFormatter.getDefinition(types[0]);
}

const requirements: Definition[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Dependencies may be a better word.

@filipomar filipomar requested a review from domoritz May 2, 2022 08:24

types.forEach((t) => {
if (t instanceof ArrayType || t instanceof TupleType) {
/**
Copy link
Member

Choose a reason for hiding this comment

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

Use // comments in code

const dependencies: Definition[] = [];
const nonArrayLikeTypes: BaseType[] = [];

types.forEach((t) => {
Copy link
Member

Choose a reason for hiding this comment

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

use for ... instead of forEach

});

if (nonArrayLikeTypes.length) {
/**
Copy link
Member

Choose a reason for hiding this comment

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

same as above

if (t instanceof ArrayType || t instanceof TupleType) {
/**
* Arrays are not easily mergeable
* So it's just easier to append their defs
Copy link
Member

Choose a reason for hiding this comment

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

I would add these comments higher up and focus more on the why than the what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I quite understand what you mean by the "why vs what". I will be moving the comments higher up in the in chain for now though.

Copy link
Member

Choose a reason for hiding this comment

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

Comments should explain why we are doing something and now what is happening in the code.

@filipomar filipomar force-pushed the array-and-tuple-intersection branch from f6112e9 to 2c2e655 Compare May 3, 2022 16:19
@domoritz domoritz merged commit 243ee05 into vega:next May 6, 2022
@filipomar filipomar deleted the array-and-tuple-intersection branch May 25, 2022 15:04
@github-actions
Copy link

馃殌 PR was released in v1.1.0 馃殌

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants