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

Polishing: use readonly arrays #591

Merged
merged 1 commit into from
Dec 1, 2018
Merged

Polishing: use readonly arrays #591

merged 1 commit into from
Dec 1, 2018

Conversation

johnsonr
Copy link
Contributor

No description provided.

@ddgenome
Copy link
Contributor

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

@ddgenome
Copy link
Contributor

Although I guess if the TypeScript compiler is not complaining…

@ddgenome
Copy link
Contributor

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
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
Copy link
Contributor

ddgenome commented Dec 1, 2018

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

Copy link
Contributor

@ddgenome ddgenome left a comment

Choose a reason for hiding this comment

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

LGTM

@ddgenome ddgenome added changelog:changed Add this issue or pull request to changed changelog section auto-merge:on-approve Auto-merge on review approvals labels Dec 1, 2018
@ddgenome ddgenome merged commit 7a37171 into master Dec 1, 2018
@ddgenome ddgenome deleted the ro-array branch December 1, 2018 20:02
atomist-bot added a commit that referenced this pull request Dec 1, 2018
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge:on-approve Auto-merge on review approvals changelog:changed Add this issue or pull request to changed changelog section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants