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

Fixes #30781 - app_value() should expect Symbol #275

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Sep 4, 2020

No description provided.

@wbclark
Copy link
Contributor Author

wbclark commented Sep 4, 2020

To be honest, this doesn't make a lot of sense to me.

My understanding is that Ruby best practices are not to enforce strict typing and instead rely on respond_to? rather than is_a? when necessary.

OK, best practices are just guidelines, but I don't see a strong reason to ignore them in this case. What is the real negative outcome if someone calls app_value('my_option') and app_value converts it to :my_option for us ?

@ehelms
Copy link
Member

ehelms commented Sep 8, 2020

I interpret it a bit more like a best practice enforcement, a linting type rule. If we want users to always pass symbols because they are more efficient and we want a consistent experience then we need to type enforce. This prevents having to remember they should be symbols when we review code as tests will simply fail. That is the value I saw.

@ekohl
Copy link
Member

ekohl commented Sep 15, 2020

One possible reason that it allowed strings was that Ruby < 2.2 had a memory leak if they were generated dynamically (according to https://stackoverflow.com/questions/16621073/when-to-use-symbols-instead-of-strings-in-ruby), though if .to_sym is called here I don't exactly know the benefit. Probably just adhering to the Robustness Principle.

This is a breaking change and I don't like doing major version bumps all the time. At most this log a deprecation but even that I really doubt. We have seen that we have abysmal test coverage in our hooks so we're very likely to only hit this at runtime. That's why I think we should not do this. We should remember to use the best practices, but don't hard error on inefficiencies.

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.

4 participants