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

Fixes #1692 - improved error for unsupported commands #1695

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

amorton
Copy link
Contributor

@amorton amorton commented Nov 13, 2024

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

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

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.
@amorton amorton requested a review from a team as a code owner November 13, 2024 03:23
Copy link
Contributor

@maheshrajamani maheshrajamani left a 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.

* The canonical list of commands the API support, any command name not listed here is not a
* command.
*/
public enum CommandName {
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@maheshrajamani maheshrajamani left a comment

Choose a reason for hiding this comment

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

LGTM

@maheshrajamani maheshrajamani merged commit fc61866 into main Nov 13, 2024
3 checks passed
@maheshrajamani maheshrajamani deleted the ajm/fix-1692-error-unsupported-commands branch November 13, 2024 16:55
@tatu-at-datastax
Copy link
Contributor

Ok would have been good to discuss this wrt #1592 / PR #1645 first.

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.

Improve the error for commands that are not supported on tables, and table commands not on collections
3 participants