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

Added possibility to give opts via symbols instead of string as key for the opts hash #365

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AlbinOS
Copy link
Contributor

@AlbinOS AlbinOS commented Feb 3, 2016

Pull request for #360.

I started writing test but it felt wrong as I was only duplicating existing test and calling methods with ':key' instead of '"key"'. Is it ok for you ? Or do you have a better idea to test theses changes in a smart way ?

@AlbinOS
Copy link
Contributor Author

AlbinOS commented Feb 5, 2016

Do you have any idea why is the build of my PR failing ? I ran all tests locally and everything passed normally.

@tlunter
Copy link
Contributor

tlunter commented Mar 22, 2016

Any reason you gave preference to the symbols approach instead of strings approach? Since strings were previously default, probably a good idea to keep that first.

@AlbinOS
Copy link
Contributor Author

AlbinOS commented Mar 29, 2016

@tlunter, not really, it's just an habit I guess. I'll modify this PR according to your remark. Thanks for the feedback !

@AlbinOS
Copy link
Contributor Author

AlbinOS commented Mar 30, 2016

@tlunter, it should be ok now.

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.

2 participants