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

fix: add default keyword argument to kredis extension attributes #1897

Merged

Conversation

dylanplayer
Copy link
Member

@dylanplayer dylanplayer commented May 9, 2024

Motivation

The arguments for many attributes in Tapioca::Dsl::Compilers::Extensions::Kredis are missing the default: keyword argument defined in the kredis gem here. This is causing the following error whenever you generate the DSLs for an application that uses the default: argument on a kredis attribute.

lib/tapioca/dsl/extensions/kredis.rb:27:in `kredis_integer`: unknown keyword: :default (ArgumentError)

Implementation

Added the default: keyword argument to all of the following kredis attribute methods:

  • kredis_string
  • kredis_integer
  • kredis_decimal
  • kredis_datetime
  • kredis_flag
  • kredis_float
  • kredis_json
  • kredis_list
  • kredis_unique_list
  • kredis_set
  • kredis_counter
  • kredis_hash
  • kredis_boolean

Tests

I did not add new tests for this change as there don't appear to be existing tests for any of the other optional arguments here and this argument would not meaningfully change the DSL output (just allows it to not error when the default: argument is provided). Willing to add tests for this if needed -- leaning on the side of following the existing pattern here.

@dylanplayer dylanplayer requested a review from a team as a code owner May 9, 2024 18:24
@dylanplayer dylanplayer closed this May 9, 2024
@dylanplayer dylanplayer reopened this May 9, 2024
@KaanOzkan KaanOzkan merged commit 6ef4feb into main May 9, 2024
48 checks passed
@KaanOzkan KaanOzkan deleted the fix/update-kredis-extension-to-have-default-kw-arguments branch May 9, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants