-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fixes #1692 - improved error for unsupported commands #1695
Conversation
Moved the CommandName and related enums to be top level as they are used in multiple places and not a sub component of the Commmand interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good, added few recommendations.
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/CommandName.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/CommandName.java
Outdated
Show resolved
Hide resolved
* The canonical list of commands the API support, any command name not listed here is not a | ||
* command. | ||
*/ | ||
public enum CommandName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enum name doesn't make sense, since it has more information than it's name, need to be renamed something like CommandInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be taken as separate PR as the name was an existing one. Created this issue for tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Moved the CommandName and related enums to be top level as they are used in multiple places and not a sub component of the Commmand interface.
Fixes #1692
Which issue(s) this PR fixes:
Fixes #
Checklist