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

Http tweaks #235

Merged
merged 8 commits into from
Feb 13, 2017
Merged

Http tweaks #235

merged 8 commits into from
Feb 13, 2017

Conversation

jnunemaker
Copy link
Collaborator

  • Drops config. Felt cumbersome for just a few http options. We may need it back, but I'd rather go slim for now until we definitely do.
  • Renames Request => Client. Felt more accurate since it deals with many requests.
  • to_json to JSON.generate. I didn't realize that to_json exists in ruby and thought that was only coming from rails which might not always be there so I switched. I later realized that to_json does seem to be there in ruby for some things, though I'm not sure it is for hash (that maybe comes from rails?). Just seemed best to be explicit and use JSON.generate.
  • made merging mount point with flipper api paths a bit more robust (uses URI for mount point and path and tries to merge them.

Config felt overly complex for what is going on here. Might bring it back but I don't think we need it just yet.
to_json is from rails which might not be around. It just happens to be here because we explicitly test against rails versions.
It isn't a single request, it is a class for making requests.
I can't find in docs where the other way was working but it was so I'm confused. I know this way works as it is old as dirt.
Before it was in two. Feels better to have client just do http stuff for now and not know json.
@jnunemaker jnunemaker self-assigned this Feb 13, 2017
@jnunemaker
Copy link
Collaborator Author

I'd still like to go farther on this class and make it a full wrapper around the api (e.g.: client.features, client.feature("search"), etc.), but calling it quits for the weekend.

@jnunemaker jnunemaker merged commit 69e8a50 into master Feb 13, 2017
@jnunemaker jnunemaker deleted the http-tweaks branch February 13, 2017 15:19
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.

1 participant