search

fog / fog-openstack-core

fog's core openstack behaviors without API and cloud provider specifics
Apache License 2.0
6 stars 8 forks source link

Question re: stringify_keys #29

Open wchrisjohnson opened 10 years ago

wchrisjohnson commented 10 years ago

We need a stringify_keys variation of some sort, and I can certainly do a quick PR to submit to fog-core. I don't see one in the fog or fog-core source, only one in the mock_data.rb file within the rackspace provider that stringifies both keys and values.

Am I missing anything, background, history, decisions, before I submit this as a new PR?

If not, any requests wrt the specific implementation approach?

geemus commented 10 years ago

I don't know that I recall specifics any more (if I ever did). What is the context that we need it for?

wchrisjohnson commented 10 years ago

We need it to stringify a bunch of keys as they come into a method in an options hash. Example here: https://github.com/fog/fog-openstack-core/blob/feature/compute/lib/fog/openstackcore/requests/compute/v2/list_flavors.rb

Instead of handling each option explicitly, we are trying to dry it up, yet explicitly whitelist only those options that are available. The stringify_keys will help ensure consistency in how options come into the method to be used. I actually expect this is be used very broadly in this refactor - we have lots of requests that have many optional parameters.

I can certainly leave it in the provider, but thought it might be generally useful.

geemus commented 10 years ago

PR sounds fine. I'd just suggest moving stringify inside of whitelist to reduce what I suspect is a large amount of duplication of that pattern across requests.

wchrisjohnson commented 10 years ago

Good idea - will do.

wchrisjohnson commented 10 years ago

Wes - did you want me to submit a PR for the stringify, or whitelist+stringify? I suspect I misread your comment...

geemus commented 10 years ago

Maybe both? ie we may eventually need stringify more broadly, so having it on it's own seems useful. But calling it inside whitelist for openstack also seems like it might be cleaner in this particular case. Make sense?

wchrisjohnson commented 10 years ago

Yeah, I definitely think having it inside whitelist makes sense. If you think there is broad usefulness for both, I think I will re-structure the PR I opened last night so the two parts are able to stand alone. Thanks for the guidance!

geemus commented 10 years ago

Sure thing, thank you!

On Thu, May 1, 2014 at 10:15 AM, Chris Johnson notifications@github.comwrote:

Yeah, I definitely think having it inside whitelist makes sense. If you think there is broad usefulness for both, I think I will re-structure the PR I opened last night so the two parts are able to stand alone. Thanks for the guidance!

Reply to this email directly or view it on GitHubhttps://github.com/fog/fog-openstack-core/issues/29#issuecomment-41919127 .