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

Idiomatic go #21

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Idiomatic go #21

wants to merge 6 commits into from

Conversation

alaypatel07
Copy link

Refactored some code that looked pythonic with self as receivers.
Added Comments for most Part of the code.

Comment worth reading in urls_handler.go :

// Get implements the method for UrlHandler.
// It would be an advantage to have methods
// implemented on urls in the response.

@meson10
Copy link
Owner

meson10 commented Oct 31, 2015

Fair point. Do you have anything in mind on how the response, for methods implemented on urls, should look like?

@alaypatel07
Copy link
Author

Yes. Infact i have a couple of points to be considered which are as follows.

  1. The current response looks like this
    Response :- {"data":{"RegisterUser":"/api/user","async_pipe":"^/async_pipe/?$","pipe":"^/pipe/?$","urls":"^/urls/?$"},"message":"","status":200}

I have only implemented POST method on the "RegisterUser" endpoint. It would be better if the response looked somewhat like this:-

Expected Response :- {"data":{"RegisterUser":{"Url":"/api/user", "Methods": ["POST"]}}, "message": "", "status": 200}

  1. However, the method OPTIONS is used exactly for that, i.e. knowing what methods are available for which particular endpoint. Hence the GlobalHandler should distinguish between request of OPTIONS method and set "Allow" header.

Please refer to the snippet I am sharing.
screenshot from 2015-10-31 23 43 10

http://www.tutorialspoint.com/http/http_methods.htm

I am wanted to implement both the points today in my branch but i am confused on how to go about it. It will take time but i am hopeful, i will be able to complete it in a week or so.

Meanwhile do share your views on the same.

PS :- I have no idea on the Travis CI build error, I am sorry if your code broke

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.

2 participants