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

Make callback defs types less restrictive #364

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

ryanwinchester
Copy link
Contributor

@ryanwinchester ryanwinchester commented Oct 15, 2018

This builds on PR #363 and probably fixes #327

Examples

I used the format

@callback post(url | term, body | term, headers | term, options | term)

when I could have used a simpler

@callback post(term, term, term, term)

for the sake of code as documentation, but we can just use the simpler version if you prefer.

Reasoning

The reasoning behind these changes is that you really can pass anything in as long as your process_* override returns the proper type.

See the return type:

@spec process_request_body(any) :: body
def process_request_body(body), do: body

@ryanwinchester
Copy link
Contributor Author

An alternative approach would be to add | any to the request struct's property's types

@edgurgel
Copy link
Owner

@ryanwinchester I like the any approach! 👍 Thanks again for all the PRs :)

@ryanwinchester
Copy link
Contributor Author

@edgurgel moved it to request struct properties' types

@ryanwinchester ryanwinchester force-pushed the bugfix/callback-defs-types branch from c5a6c5c to bd14138 Compare October 16, 2018 16:58
@type params :: map | keyword | [{binary, binary}]
@type options :: keyword
@type headers :: [{atom, binary}] | [{binary, binary}] | %{binary => binary} | any
@type url :: binary | any
Copy link
Owner

Choose a reason for hiding this comment

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

now that I'm reading this we will accept any for the result of process_request_url right? 🤔

Copy link
Contributor Author

@ryanwinchester ryanwinchester Oct 16, 2018

Choose a reason for hiding this comment

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

Technically we want anything that implements String.Chars protocol (i.e. can work with to_string/1), but we can't really spec for protocols, so... we're kinda forced to give suggestions, and then include any on the end, imo

Same could be said for some of the other properties, too.

For example the :body we can expect {:stream, enumerable} but can't really spec for the Enumerable protocol, so... any :/

Anyways, they can technically pass any where any of the properties have an associated process_x function and they implement it and return the expected type.

We could make it more obvious if we do:

@callback process_request_url(any) :: url

@spec process_request_url(any) :: url
def process_request_url(url), do: process_url(url)

instead of

@callback process_request_url(url) :: url

@spec process_request_url(url) :: url
def process_request_url(url), do: process_url(url)

(even though they'd really be the same effect in the end, it's just visual)

Thoughts?

Copy link
Contributor Author

@ryanwinchester ryanwinchester Oct 16, 2018

Choose a reason for hiding this comment

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

@edgurgel I think it's either this or telling people to cast to string first and close #327 (and possibly tag wontfix)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I agree with any for the input. I'm wondering about the return being url as any is also acceptable 🤔

Copy link
Contributor Author

@ryanwinchester ryanwinchester Oct 16, 2018

Choose a reason for hiding this comment

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

@edgurgel with this change url includes | any ;)

So really we can put either, but i feel like using url in the return conveys the intention better, even though it doesn't really matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, I'm fine either way, just let me know which you want and where.

Copy link
Owner

Choose a reason for hiding this comment

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

That's fine then 👍

@edgurgel edgurgel merged commit 8fb79a7 into edgurgel:master Oct 16, 2018
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.

Overly restrictive url typespec
2 participants