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

feat(collections): compile time guarantee on pure functions #1119

Merged
merged 4 commits into from
Aug 12, 2021

Conversation

zhangyuannie
Copy link
Contributor

Before 😕

const arr = Object.freeze([1, 2, 3, 4, 5]);
const a = findLast(arr, (e) => e === 1);
                   ^^^
// Argument of type 'readonly number[]' is not assignable to parameter of type 'number[]'.
//   The type 'readonly number[]' is 'readonly' and cannot be assigned to the mutable type 'number[]'.

After 😃

const arr = Object.freeze([1, 2, 3, 4, 5]);
const a = findLast(arr, (e) => e === 1);

On top of that, the compiler will also help std maintainers to not mutate the parameters by mistake to a certain degree.

export function foo(array: readonly number[]) {
  array.push(1);
        ^^^^
  // Property 'push' does not exist on type 'readonly T[]'.

  array[2] = 3;
  ^^^^^^^^
  // Index signature in type 'readonly number[]' only permits reading.

  ...
}

@LionC
Copy link
Contributor

LionC commented Aug 11, 2021

This has actually been on my to do list - looks like I can check that off. Thank you!

I will do a review later today.

Copy link
Contributor

@LionC LionC left a comment

Choose a reason for hiding this comment

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

Looks good except the one minor thing (which is not your fault but could be easily fixed here) and one question.

Thanks for doing the tedious work to go through this, much appreciated!

collections/deep_merge.ts Outdated Show resolved Hide resolved
collections/deep_merge.ts Outdated Show resolved Hide resolved
collections/zip.ts Outdated Show resolved Hide resolved
@LionC
Copy link
Contributor

LionC commented Aug 12, 2021

LGTM! Pinging @kt3k for final approval :-)

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.

@zhangyuannie LGTM. Thanks! Nice improvement 👍

And thanks @LionC for reviewing!

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