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.

Sign up for GitHub

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

Jump to bottom

Rename HTTP::Params.from_hash to HTTP::Params.encode and add an overload for NamedTuple #4205

Merged
merged 1 commit into from Apr 20, 2017
Merged

Rename HTTP::Params.from_hash to HTTP::Params.encode and add an overload for NamedTuple #4205

merged 1 commit into from Apr 20, 2017

Conversation

jhass
Copy link
Member

@jhass jhass commented Mar 28, 2017

Also allow passing NamedTuple to the corresponding post_form overloads
on HTTP::Client

That should make passing some static form data (or even some data with static keys) more pleasant without sacrificing on performance as much.

luislavena
luislavena reviewed Mar 28, 2017
View reviewed changes
src/http/client.cr Outdated
@@ -418,8 +418,8 @@ class HTTP::Client
# client = HTTP::Client.new "www.example.com"
# response = client.post_form "/", {"foo" => "bar"}
# ```
def post_form(path, form : Hash, headers : HTTP::Headers? = nil) : HTTP::Client::Response
body = HTTP::Params.from_hash(form)
def post_form(path, form : Hash|NamedTuple, headers : HTTP::Headers? = nil) : HTTP::Client::Response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format check failed, I believe spaces are required on type unions (Hash | NamedTuple)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, got a bit rusty I guess :)

@RX14
Copy link
Contributor

RX14 commented Mar 28, 2017

.from seems like a little bit of a weird name to me. Maybe .of? That's a little bit weird too i guess. 🤔

@luislavena
Copy link
Contributor

@RX14 I find .from to be consistent with others:

  • json: .from_json
  • yaml: .from_yaml
  • crystal-db: .from_rs

I think in this case, _hash and _tuple are implicit and described by the parameter given to the method call.

@RX14
Copy link
Contributor

RX14 commented Mar 28, 2017

@luislavena The consistency between them is that they have the type it's coming from in the same. They sound fine, from on it's own feels weird to me. Why not 2 seperate methods: from_hash and from_tuple?

@asterite
Copy link
Member

What about new?

@jhass
Copy link
Member Author

jhass commented Mar 28, 2017 •
edited

Both of and new even stronger hint at returning an instance of HTTP::Params, while it returns String.

How about we overload build instead?

@asterite
Copy link
Member

Oooh... I thought it returned an HTTP::Params. I don't know of a good name, then. Maybe to_s.

@jhass
Copy link
Member Author

jhass commented Mar 28, 2017

Or just encode?

@jhass jhass changed the title Rename HTTP::Params.from_hash to HTTP::Params.from and add an overload for NamedTuple Rename HTTP::Params.from_hash to HTTP::Params.encode and add an overload for NamedTuple Mar 28, 2017
bcardiff
bcardiff reviewed Mar 29, 2017
View reviewed changes
src/http/params.cr Outdated
def self.from_hash(hash : Hash)
# Returns the given key value pairs as a
# url-encoded HTTP form/query.
def self.encode(hash : Hash)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should restrict to Hash(String, _). I know it isn't part of the initial PR intent.

If we want to work with any type of hash we can merge the implementation.
If we want to support only String key hashes we could restrict here and catch the compile time error sooner.

Hash(String, _) sounds fine to me.

@jhass
Rename HTTP::Params.from_hash to HTTP::Params.encode and add an overload
3bacc30 
for NamedTuple

Also allow passing NamedTuple to the corresponding post_form overloads
on HTTP::Client
@jhass
Copy link
Member Author

jhass commented Apr 2, 2017

Alright, added the more specific restriction.

@Sija
Copy link
Contributor

Sija commented Apr 15, 2017

Is this PR GTG?

@asterite asterite merged commit 919bc4c into master Apr 20, 2017
@asterite asterite added this to the Next milestone Apr 20, 2017
@straight-shoota straight-shoota mentioned this pull request Aug 1, 2017
Merged
jwoertink added a commit to jwoertink/crystal that referenced this pull request Aug 1, 2017
@jwoertink
fixing method call on HTTP::Params to fix failing specs. from_hash wa…
48e7a8b 
…s renamed to encode in crystal-lang#4205
asterite pushed a commit that referenced this pull request Aug 22, 2017
@jwoertink @asterite
fixing method call on HTTP::Params to fix failing specs. from_hash wa…
66ad88f 
…s renamed to encode in #4205
@asterite asterite deleted the feature/http_params_from branch April 27, 2018 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luislavena luislavena luislavena left review comments

@bcardiff bcardiff bcardiff left review comments

Assignees
No one assigned
Labels
kind:refactor topic:stdlib
Projects
None yet
Milestone
0.22.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jhass @RX14 @luislavena @asterite @Sija @bcardiff