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 missing defaults #689

Merged
merged 1 commit into from Nov 17, 2020
Merged

Conversation

aeroastro
Copy link
Contributor

I would like to add some missing keys in DEFAULTS so that options with string keys are accepted.

Also, I added a comment in order not to forget to add keys.

Since Ruby 2.2 and after has symbol GC functionality, it might be about time to convert all string keys to symbols.

c.f. #256 #259

@aeroastro aeroastro force-pushed the feature/add-missing-defaults branch 2 times, most recently from e94b37d to 83f366d Compare April 26, 2017 06:05
@aeroastro
Copy link
Contributor Author

Hello.

It's been a while since I submitted this PR, and I'm wondering if there is anything I could do on my end. I totally understand you are busy and require more time to consider, it is much appreciated if you could give me a response.

Also, IMHO, memory leak pointed out here #256 (comment) is becoming a very minor issue and I prefer converting all the keys from String to Symbol.

If it is O.K. I will fix my patch!

Here are the reasons.

  1. Rational developers won't pass hashes containing a large number of unsupported keys to external libraries as their options, especially on environments where memory leak is a problem.
  2. In most cases, unintentionally inserted keys in options will not change dynamically until the program ends, which means unintentionally used memory size by converted Symbols is limited (fixed).
  3. Even if some developers pass the options containing a large number of dynamically-changing unsupported keys, currently (September, 2017) supported ruby versions, which is 2.2 and later, have Symbol GC functionality and memory leak is restrained to some extent.

@aeroastro
Copy link
Contributor Author

@pietern

Could you take a short look into this PR?
I think it is about time to convert all keys from String to Symbol.

@aeroastro
Copy link
Contributor Author

@byroot
I have completed adjusting the commit to the latest master branch.
Could you take a look into this?

@byroot byroot merged commit d896ae2 into redis:master Nov 17, 2020
@aeroastro aeroastro deleted the feature/add-missing-defaults branch November 18, 2020 05:00
@aeroastro
Copy link
Contributor Author

Thank you ⭐

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.

None yet

2 participants