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

Added Rawable interface #2370

Merged
merged 6 commits into from
Mar 17, 2021
Merged

Added Rawable interface #2370

merged 6 commits into from
Mar 17, 2021

Conversation

sazzad16
Copy link
Collaborator

@sazzad16 sazzad16 commented Feb 7, 2021

Jedis should have a common interface to represent all commands and keywords (and, perhaps, arguments). Closest thing we currently have is ProtocolCommand interface. But the problem is that it contains the word COMMAND which would be misleading to use for something other than commands.

One option is to rename the existing interface. But this would be a breaking change and so it can't be available before at least Jedis 4.0. Even that can be received negatively by the users who could be using the existing interface.

Having a parent interface, on the other hand, does not have such issues and can be included in upcoming non-major release.

I went with name Raw, instead of other names like ProtocolKeyword or ProtocolArgument, because it is short and fits nicely with our go to method getRaw().

I have also put it in a separate package because with growing number of classes, we'd be able to put some classes in that package.

Copy link
Contributor

@mina-asham mina-asham left a comment

Choose a reason for hiding this comment

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

Do you have a future use of this in mind?

@sazzad16
Copy link
Collaborator Author

Yeah. Will do it later.

@sazzad16 sazzad16 marked this pull request as ready for review March 15, 2021 08:02
@sazzad16 sazzad16 added this to the 3.6.0 milestone Mar 15, 2021
@sazzad16
Copy link
Collaborator Author

A recent example is #2424, where UnblockType could implement the common interface.

Copy link
Collaborator

@dengliming dengliming 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. Do we have a better naming option than Raw for the interface? 😃

@sazzad16
Copy link
Collaborator Author

Do we have a better naming option than Raw for the interface?

@dengliming There are many other options.

  • ProtocolArgument
  • ProtocolKeyword
  • RawArgument
  • and many more

Is any of these better? I'm not sure. 😆

@dengliming
Copy link
Collaborator

Not sure. but I prefer ProtocolKeyword. : )

dengliming
dengliming previously approved these changes Mar 15, 2021
Copy link
Collaborator

@dengliming dengliming left a comment

Choose a reason for hiding this comment

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

Raw and ProtocolKeyword are both good to me. : )

@gkorland
Copy link
Contributor

I like Raw I think it captures the functionality of it, or if we want to be more "Java" Rawable

@sazzad16
Copy link
Collaborator Author

@dengliming What are your preferences now? (including Rawable)

@dengliming
Copy link
Collaborator

Rawable

+1 : )

@sazzad16 sazzad16 changed the title Added Raw interface Added Rawable interface Mar 17, 2021
@sazzad16 sazzad16 merged commit 3a3b89a into redis:master Mar 17, 2021
@sazzad16 sazzad16 deleted the raw-interface branch March 17, 2021 13:15
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.

4 participants