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

Typified command list on compound command. Cleaned-up compound command. #239

Merged
merged 1 commit into from
Aug 13, 2023

Conversation

azoitl
Copy link
Contributor

@azoitl azoitl commented Aug 13, 2023

Addresses #155

@Destrolaric @ptziegler as it is only changing GEF internals it shouldn't have any impact on users, or?

@github-actions
Copy link

github-actions bot commented Aug 13, 2023

Unit Test Results

    9 files      9 suites   18s ⏱️
307 tests 305 ✔️ 2 💤 0
921 runs  915 ✔️ 6 💤 0

Results for commit c15cfe1.

♻️ This comment has been updated with latest results.

@ptziegler
Copy link
Contributor

as it is only changing GEF internals it shouldn't have any impact on users, or?

The only part that might cause problems is the type returned by getCommands(). Currently, something like this is currently valid, but would cause errors after this change:

List<Object> commands = compoundCommand.getCommands();

Though I think that this is a rather theoretical example, because I don't see why one would want to store the value like this rather than a raw list or directly as a generic list of commands.

@azoitl
Copy link
Contributor Author

azoitl commented Aug 13, 2023

I missed that. I have this change for some time in my backlog. The better solution is to return an < ? extends Command> this gives existing clients more flexibility. Will change it.

@ptziegler
Copy link
Contributor

I missed that. I have this change for some time in my backlog. The better solution is to return an < ? extends Command> this gives existing clients more flexibility. Will change it.

I don't think that's a good idea. Then you can only store the returned value as a List<? extends Command>, but no longer as a List<Command>.

@ptziegler
Copy link
Contributor

I don't think that's a good idea. Then you can only store the returned value as a List<? extends Command>, but no longer as a List<Command>.

Whereas with with List<Command>, you can store it as both.

@azoitl
Copy link
Contributor Author

azoitl commented Aug 13, 2023

Thx for the feedback. Will merge it now.

@azoitl azoitl merged commit eb1d17a into eclipse-gef:master Aug 13, 2023
@azoitl azoitl deleted the Compound_command branch August 13, 2023 16:53
@azoitl azoitl added this to the 3.17.0 milestone Aug 13, 2023
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