search

inhabitedtype / ocaml-webmachine

A REST toolkit for OCaml
Other
221 stars 31 forks source link

Post create #96

Closed linse closed 4 years ago

linse commented 5 years ago

Adding missing methods post_is_create and create_path.

seliopou commented 4 years ago

I'm finally getting around to looking at this.

seliopou commented 4 years ago

So it looks like this doesn't fully implement POST support, as it's missing a few decision points and doesn't actually make use of allow_missing_post which is still commented out. I don't know if it's safe to just modify n11 without considering all the other decision points.

hannesm commented 4 years ago

Hi Spiros,

thanks for looking into this PR.

So it looks like this doesn't fully implement POST support, as it's missing a few decision points and doesn't actually make use of allow_missing_post which is still commented out.

We just implemented the parts we needed so far, the code for "allow_missing_post" can be added when needed. We are using this PR in an internal project, and it works really well. There, we have tests for create_path and post_is_create.

I don't know if it's safe to just modify n11 without considering all the other decision points

We followed the Erlang implementation, n11 is inspired by https://github.com/webmachine/webmachine/blob/1.11.1/src/webmachine_decision_core.erl#L459 A test case for n11 is part of this PR.

Which other decision points do you think need to be considered? Can you give us a list so we don't miss anything?

seliopou commented 4 years ago

I'd like for any merged PR that adds POST functionality to cover any and all decision points that involve the following callbacks:

@tmcgilchrist started implementing some of these in https://github.com/inhabitedtype/ocaml-webmachine/commit/6ff58d817e065eff5eefd3980880c1a26c41a17c. I don't know to what extent that patch covers all the decision points but it's a start.

linse commented 4 years ago

Our changes were orthogonal to @tmcgilchrist 's. We combined them, ported the according tests from Erlang and fixed a bug. We think it's good to go now.

seliopou commented 4 years ago

I can't seem to push to the pr branch. Could you enable that?

linse commented 4 years ago

I can't seem to push to the pr branch. Could you enable that?

I think you already pushed a commit, and I can't find an option to enable pushing specifically. You could also open a new PR to this branch if you are still stuck.

Thanks for reviewing! Do you need anything else from us or is this good to go?

seliopou commented 4 years ago

That commit was added via GitHub's web interface. When I try to push I get the following error:

$ git push roburio post-create 
ERROR: Permission to roburio/ocaml-webmachine.git denied to seliopou.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
hannesm commented 4 years ago

@seliopou that is strange, we're missing the checkbox to "allow edits from maintainers"; I can see two options: use a branch on your fork (and open a new PR), or PR to our repository.

hannesm commented 4 years ago

@seliopou to unblock you on this PR, I invited you with write permissions to roburio/ocaml-webmachine. when you accept that invite, you should be able to push to the branch and complete this PR.

hannesm commented 4 years ago

dear @seliopou, a friendly ping on this issue. it would be nice to get this into webmachine-HEAD, and get a release to opam-repository. if you need anything from us, please let us know. thanks.

seliopou commented 4 years ago

Sorry forgot about this. I'm gonna do a pass of all my open source libs this weekend, and will look at it then.