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

std/net/request: allow attaching data to all http verbs #603

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

yanndegat
Copy link

export http-request for low level http usage.

@vyzo
Copy link
Collaborator

vyzo commented Mar 12, 2021

Note sure that's a good idea, it kind of breaks the abstraction barrier.
What are you trying to do that needs it?

@yanndegat
Copy link
Author

well i'm struggling with the bitbucket apis, which can send data for delete verbs.

this kind of apis, which not necessarily respects REST conventions, exist almost every where
maybe we could expose all http-request options in all http-get/post/put/delete/patch func ?

@vyzo
Copy link
Collaborator

vyzo commented Mar 12, 2021

Yeah, that makes more sense.
Let's do that instead, I don't want to leak the internals of the http-request type as this will make it hard to change it without breaking user code.

@yanndegat yanndegat force-pushed the export-http-request branch from 500bd33 to 8308805 Compare March 12, 2021 16:44
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

LGTM

@yanndegat
Copy link
Author

Yeah, that makes more sense.
Let's do that instead, I don't want to leak the internals of the http-request type as this will make it hard to change it without breaking user code.

ok done.

now:

all methods (get,head,delete,put,options) have the exact same impl, apart from the VERB

(btw options & head use the same verb. shouldnt it be 'OPTIONS in the http-option fun?)

@vyzo vyzo changed the title std/net/request: export method http-request std/net/request: allow attaching data to all http verbs Mar 12, 2021
@vyzo
Copy link
Collaborator

vyzo commented Mar 12, 2021

btw options & head use the same verb. shouldnt it be 'OPTIONS in the http-option fun?

duh yes, that's a bug!

@vyzo
Copy link
Collaborator

vyzo commented Mar 12, 2021

care to fix in this pr?

make http-* take the same args list
(http-get, http-delete, http-options)
@yanndegat yanndegat force-pushed the export-http-request branch from 8308805 to 2bc526a Compare March 12, 2021 16:52
@yanndegat
Copy link
Author

done

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

thank you!

@vyzo vyzo merged commit e7c0fee into mighty-gerbils:master Mar 12, 2021
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.

3 participants