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

Add marshal for request is a x-www-form-urlencoded and response is a json #548

Closed
wants to merge 2 commits into from

Conversation

jinleileiking
Copy link

@jinleileiking jinleileiking commented Feb 13, 2018

#528
#7

After 2 days reading the source code. I found a better way for this problem.

The full curl is :

➜  ~ git:(master) ✗ curl '10.64.7.106:8933/form'  -d 'str=333&int=444' -XPOST -H 'Content-Type: application/x-www-form-urlencoded'
{"errno":444,"errmsg":"333"}

@jinleileiking
Copy link
Author

Can this be merged?

@jinleileiking
Copy link
Author

@yugui @tmc

@jinleileiking
Copy link
Author

@achew22

@achew22
Copy link
Collaborator

achew22 commented Mar 6, 2018

@jinleileiking, thanks so much for your efforts so far! This seems like an interesting marshaller to add, there are just a few things that need to be done before it can be considered for inclusion in the mainline repo.

  • Unit Tests of the file in question
  • Documentation of how one would use this new functionality
  • Coverage in end to end tests (to ensure I don't accidentally make your code regress when doing a refactoring later)

In the meantime, since the marshaller is a pluggable part of the system I would encourage you to bring this code into the mainline of your project to get you un-stuck.

@jinleileiking
Copy link
Author

@achew22 Thank you. I will add these doc, ut,cov things.

Suggestion:

Shall we add a marshal-contrib project to make these non-official marshaler easy to plug?

@johanbrandhorst
Copy link
Collaborator

Looks like we're still missing tests and documentation here.

@johanbrandhorst
Copy link
Collaborator

Please rebase on master for new CI functionality.

@mohammed90 mohammed90 mentioned this pull request Jul 6, 2019
9 tasks
@johanbrandhorst
Copy link
Collaborator

Stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants