scorelab / Stackle

Stackle is an web communication portal aimed at providing Open Source organizations a platform to have discussions on their github projects and their issues. It provides Github intergration which allows adminstrator of an organization to create a forum thread for the particualr organization. Users signing in is able to view forums of the organizations they contribute to and engage in the forum discussions.
Apache License 2.0
104 stars 146 forks source link

Modularize api #88

Closed bhavyaagg closed 6 years ago

bhavyaagg commented 6 years ago

I have modularized the API routes. But i think there are some parts which can be improved:

1) HTTP verbs should not be present in the URL https://github.com/scorelab/Stackle/blob/master/stackle_api/app/routes.js#L148

HTTP verb "delete" is also used in the URL

2) Singular and Plural both are used for an object in the URL i.e. We have used /posts as well as /post. https://github.com/scorelab/Stackle/blob/master/stackle_api/app/routes.js#L25 https://github.com/scorelab/Stackle/blob/master/stackle_api/app/routes.js#L49

It should be /api/posts and /api/posts/:postid respectively

Same for orgs and org

3) https://github.com/scorelab/Stackle/blob/master/stackle_api/app/routes.js#L160 This can be /api/stack/subscribe

These are just my opinions. I know I might be wrong. But would love to work on Stackle :)

bhavyaagg commented 6 years ago

Should we use single quote(') or double quote(") in the project?

psnmissaka commented 6 years ago

Good job @bhavyaagg on trying to Modularize the API. It was actually a task planned for GSOC as well. Now we could save some time for that.

However i think we must keep the org related routes in one file and post related routes in another file rather than having two more route files for orgs and posts

On your previous queries:

  1. I agree with not having HTTP verbs in the API endpoint URL, it's if better we can update the API end points like that going forward.
  2. The reason of having both singular and plural object names was to differentiate API calls to a single post and for all posts.
  3. Yes. We have to change it. end point naming will be consistent.

We are planning on following Airbnb style guide for Stackle. So it's better to always use single quotes and backticks as necessary. It's also encouraged to use ES6 syntax.

bhavyaagg commented 6 years ago

@psnmissaka Thank you for reviewing my PR. Sorry I am not well today. Will update my PRs tomorrow morning. I think we can use eslint also. Would add that too.

For the plural case I think we can do /posts/:postid for single post i guess?

agentmilindu commented 6 years ago

@bhavyaagg I agree on both 1 and 2! I'm not sure about 3, but it sounds good.

agentmilindu commented 6 years ago

@padamchopra @iammosespaulr Please review.

psnmissaka commented 6 years ago

@bhavyaagg I did some reading and yes what you suggested seems like the better way to do it. Let's follow a resource naming style like /api/users for resource collections and /api/users/:userId for single resource.

iammosespaulr commented 6 years ago

Also we have this one

Conflicting files
stackle_api/server.js
bhavyaagg commented 6 years ago

I have updated my PR

bhavyaagg commented 6 years ago

@padamchopra I have updated the PR

bhavyaagg commented 6 years ago

I have updated the PR to incorporate the necessary changes

bhavyaagg commented 6 years ago

@psnmissaka @padamchopra @agentmilindu Can this be merged?

bhavyaagg commented 6 years ago

@psnmissaka @agentmilindu I have tested the API Endpoints as well.