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

Polishing: use readonly arrays #591

Merged
merged 1 commit into from Dec 1, 2018

Conversation

Projects
None yet
2 participants
@johnsonr
Copy link
Contributor

johnsonr commented Nov 30, 2018

No description provided.

@ddgenome

This comment has been minimized.

Copy link
Member

ddgenome commented Nov 30, 2018

Won't this break the implementations of, for example, addExtentionPacks since it modifies the extensionPacks array?

@ddgenome

This comment has been minimized.

Copy link
Member

ddgenome commented Nov 30, 2018

Although I guess if the TypeScript compiler is not complaining…

@ddgenome

This comment has been minimized.

Copy link
Member

ddgenome commented Nov 30, 2018

It's strange that the interface defines ReadOnlyArrays

readonly startupListeners: ReadonlyArray<StartupListener>;

but the compiler allows the implementation to use regular arrays that can be mutated

public readonly startupListeners: StartupListener[] = [];

and are mutated

@johnsonr

This comment has been minimized.

Copy link
Contributor Author

johnsonr commented Dec 1, 2018

I assume that the implementation is seeing the mutable type. Whereas callers through the interface are constrained, which is the goal.

@ddgenome

This comment has been minimized.

Copy link
Member

ddgenome commented Dec 1, 2018

So the interface constrains the consumer and the implementer different ways. Intriguing.

@ddgenome
Copy link
Member

ddgenome left a comment

LGTM

@ddgenome ddgenome merged commit 7a37171 into master Dec 1, 2018

1 of 2 checks passed

sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals in progress
Details
license/cla Contributor License Agreement is signed.
Details

@ddgenome ddgenome deleted the ro-array branch Dec 1, 2018

atomist-bot added a commit that referenced this pull request Dec 1, 2018

Changelog: #591 to changed
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment