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

Made array arguments readonly. #78

Merged
merged 3 commits into from
Jul 11, 2021
Merged

Made array arguments readonly. #78

merged 3 commits into from
Jul 11, 2021

Conversation

geon
Copy link
Contributor

@geon geon commented Jul 5, 2021

The array arguments have no business being mutable.

Purpose

The assertion functions can't be called with ReadonlyArray. That is bad.

Approach

Make the arguments readonly.

Learnings

https://www.typescriptlang.org/docs/handbook/interfaces.html#readonly-properties

The array arguments have no business being mutable.
Copy link
Collaborator

@Aboisier Aboisier 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 your contribution!

Would you mind adding two tests in order to document each use case (calling arraysToBeEqual and toBeIn with a readonly array) and prevent potential regressions?

There are already a few tests here and here.

@geon
Copy link
Contributor Author

geon commented Jul 9, 2021

@Aboisier

I don't know how I could add tests for this. The change affects only the typing. It has no runtime impact.

A test of this would have to involve running the compiler on invalid code and see if it gives the correct error. Probably outside the scope of this project.

@Aboisier
Copy link
Collaborator

Aboisier commented Jul 11, 2021

@geon

Indeed, the test I had in mind was a happy-path scenario (calling the methods with a readonly array) and would simply prevent accidental changes to the interface.

Copy link
Collaborator

@Aboisier Aboisier left a comment

Choose a reason for hiding this comment

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

LGTM!

@Aboisier Aboisier merged commit 698d4a2 into Testy:main Jul 11, 2021
@Aboisier
Copy link
Collaborator

Aboisier commented Jul 12, 2021

This has now been published in the latest version,1.5.0. Thanks for your contribution, it is really appreciated, @geon !

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.

2 participants