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

Support for return values other than ints with backwards-compatability #123

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

SerenaLynas
Copy link

This PR allows commands to return any object. Because of Java's autoboxing, it is mostly backwards compatible with older code which returns ints. The only breaking changes are that old user code which references Command<S> must be changed to Command<S, Integer>, and the return value from command execution is now Object, so the type needs to be checked/cast to int. Please let me know if there are any other breaking changes. I didn't change the tests aside from adding the Integer generic as mentioned above, so everything else should be the same.

I think that many users of this library will find the minimal cost of refactoring worth it given the immense benefit of returning more than just ints.

M4ximumPizza
M4ximumPizza previously approved these changes Dec 4, 2022
Copy link

@M4ximumPizza M4ximumPizza 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. I would say it would be approved.

For all boxed primitive numeric types, combine now adds them instead of just integers
@SerenaLynas
Copy link
Author

SerenaLynas commented Dec 4, 2022

I made minor changes to the new parts in this PR. No other changes were made since @M4ximumPizza's review.

  • Default behavior for all numeric boxed primitives (int, short, double, etc.) is now to add them, not just ints. This makes the behavior a bit more consistent across numeric types.
  • I added overloads for the execute method on CommandDispatcher which allow users to optionally specify a "base" commandResult for the other results to be combined into, ex. to get results in a list: subject.execute(parsed, new ListCommandResult())

TODO: write tests for the new functionality.

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