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

Add support for list default value #73

Closed
wants to merge 1 commit into from

Conversation

mwsteb
Copy link

@mwsteb mwsteb commented Feb 24, 2022

First of all, thanks for a great gem!

Here's my attempt at implementing #66.

I wasn't sure how strict to be about default value type. For now, I opted for giving the user scissors and hoping they won't run with them.

Hope you don't mind I butted in like that, @tleish! Not sure if you started to work on this or not.

closes: #66

@tleish
Copy link
Contributor

tleish commented Feb 25, 2022

@mwsteb - I don't mind at all. While my example in #66 shows kredis_list, I want this with other (if not all) Kredis types. I might suggest adding more default types to this PR, or removing closes: #66 from the PR comment so additional PRs can be added with default types.

@mwsteb
Copy link
Author

mwsteb commented Mar 2, 2022

Makes sense. Since it's not a lot of code and it's all related, I can do it all in this PR.

I'm not sure about some types:

  • Proxy - unclear what this type is exactly and how it's used and what a typical default value would be
  • Cycle - setting the default value can be achieved by manipulating the order of the list - eg. Kredis.cycle "mycycle", values: %i[ three one two ]. Is adding an additional default option worth it?
  • Slot(s) - seems a bit contrived to be using default value for this type, but maybe I just lack imagination :)

@tleish
Copy link
Contributor

tleish commented Mar 2, 2022

Proxy is a base class, so I don't think you need to worry about it. I assume it's safe to skip defaults for the other types as well for now.

@dhh
Copy link
Member

dhh commented Jun 18, 2022

Superseded by #89

@dhh dhh closed this Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Default proc if empty
3 participants