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 warning instance variable uninitialized #2535

Merged

Conversation

akmhmgc
Copy link
Contributor

@akmhmgc akmhmgc commented Aug 14, 2022

Summary

Fixed the warning instance variable @flexible_key is not initialized

Other Information

Running the tests, I found warning down below and fix it : )

/faker/lib/faker.rb:191: warning: instance variable @flexible_key not initialized

Copy link
Contributor

@thdaraujo thdaraujo left a comment

Choose a reason for hiding this comment

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

thanks for reporting this, good catch!

I wonder if we could refactor the usage of flexible to make it more clear. It's a bit obscure right now.

Maybe we should have an attr_reader :flexible_key instead? The advantage is that we wouldn't have to worry about @flexible_key being defined anymore.

The other advantage is that these types of tests here could just check the value of @tester.flexible_key as well, instead of having to access the internal variable:

flexible_key = @tester.instance_variable_get('@flexible_key')

What do you think?

lib/faker.rb Outdated
@@ -188,7 +188,7 @@ def flexible(key)
# girls_name: ["Alice", "Cheryl", "Tatiana"]
# Then you can call Faker::Name.girls_name and it will act like #first_name
def method_missing(mth, *args, &block)
super unless @flexible_key
super unless defined? @flexible_key
Copy link
Contributor

Choose a reason for hiding this comment

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

have you considered the case where @flexible_key is defined, but its value is set to nil?
This would probably raise an error below.

Copy link
Contributor Author

@akmhmgc akmhmgc Aug 17, 2022

Choose a reason for hiding this comment

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

Oh, I forgot about that...

@foo = nil
p 'foo is defined' if defined? @foo
p 'foo is not nil' if @foo
#=> "foo is defined"

As you said, my change raise the error subclass defined even when @flexible_key is set to nil.

@akmhmgc akmhmgc force-pushed the fix_uninitialized_instance_warning branch from ac2078b to 9c3df0f Compare August 17, 2022 06:18
@akmhmgc
Copy link
Contributor Author

akmhmgc commented Aug 17, 2022

@thdaraujo

Maybe we should have an attr_reader :flexible_key instead? The advantage is that we wouldn't have to worry about @flexible_key being defined anymore.

I think it's better way too!!
Please check this when you have a time : )
9c3df0f

@akmhmgc akmhmgc requested a review from thdaraujo August 17, 2022 06:31
@Zeragamba Zeragamba merged commit 2f3e2a1 into faker-ruby:master Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants