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

[Redis 6.2] Add MINID and LIMIT options to xtrim #1170

Merged
merged 1 commit into from
Dec 4, 2022

Conversation

wcmonty
Copy link
Contributor

@wcmonty wcmonty commented Nov 28, 2022

Adds the following options to the xtrim command:

  • Set the strategy to MINID
  • Set the limit option

This probably isn't the ideal interface for the #xtrim method, but I tried not to add breaking changes.

# Interface in this PR
xtrim(key, len_or_id, strategy: 'MAXLEN', approximate: false, limit: nil)

# Possible alternative interface (but requires a breaking change)
xtrim(key, maxlen: nil, minid: nil, approximate: false, limit: nil)

See the documentation at https://redis.io/commands/xtrim/

References #978

Comment on lines 86 to 89
strategy = strategy.to_s.upcase
unless %w[MAXLEN MINID].include?(strategy)
raise ArgumentError, "strategy must be MINID or MAXLEN"
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced all this validation is worth it, might as well directly pass it to the server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 5d7ceea

raise ArgumentError, "strategy must be MINID or MAXLEN"
end

args = [:xtrim, key, strategy, (approximate ? '~' : nil), len_or_id].compact
Copy link
Collaborator

@byroot byroot Nov 28, 2022

Choose a reason for hiding this comment

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

Suggested change
args = [:xtrim, key, strategy, (approximate ? '~' : nil), len_or_id].compact
args = [:xtrim, key, strategy]
args << '~' if approximate
args << len_or_id

I know the code was like this, but I'd rather add to the array only when necessary than to compact nil elements. It's less error prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, we are going to have to either compact/handle the nils in this test case or change the behavior:
https://github.com/redis/redis-rb/blob/master/test/lint/streams.rb#L131 . If we don't then, the we receive a TypeError with this message Unsupported command argument type: NilClass.

A couple of options:

  • Change the behavior so that we throw the TypeError instead of a Redis::CommandError
  • Leave as-is, and compact one
  • Combine your suggestion above with a compact before we call send_command

What's your preference?

Copy link
Collaborator

Choose a reason for hiding this comment

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

or change the behavior:
https://github.com/redis/redis-rb/blob/master/test/lint/streams.rb#L131

Yeah, that is inconsistent with the rest of the library, just remove that part of the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and squashed into 5d7ceea

@wcmonty wcmonty force-pushed the wm/add_minid_option_to_xtrim branch from ffc4d3d to 5d7ceea Compare December 4, 2022 15:35
args << '~' if approximate
args << len_or_id
args.concat(['LIMIT', limit]) if limit
send_command(args.compact)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
send_command(args.compact)
send_command(args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that slipped through. Updated in 4321159

@wcmonty wcmonty force-pushed the wm/add_minid_option_to_xtrim branch from 5d7ceea to 4321159 Compare December 4, 2022 15:46
@byroot
Copy link
Collaborator

byroot commented Dec 4, 2022

The CI failures seem legit. I assume there another test passing nil as argument somewhere.

@wcmonty
Copy link
Contributor Author

wcmonty commented Dec 4, 2022

So the CI failures were because I expected that the ensure would be skipped in cases where were were skipping the spec. Seems like that's common enough to have it's own Rubocop cop. I pushed up d7926fe. If that looks good, I'll squash the changes.

@wcmonty wcmonty force-pushed the wm/add_minid_option_to_xtrim branch from d7926fe to e76e1cb Compare December 4, 2022 23:01
@wcmonty
Copy link
Contributor Author

wcmonty commented Dec 4, 2022

Alright, I squashed the commits. Thanks for working with me on this.

@byroot byroot merged commit 5a9fb00 into redis:master Dec 4, 2022
@wcmonty wcmonty deleted the wm/add_minid_option_to_xtrim branch December 5, 2022 00:30
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