usgo / online-ratings

AGA Online Ratings protocol and implementation
MIT License
23 stars 12 forks source link

Tournament player1 #64

Closed TheObtuseAutodidact closed 8 years ago

TheObtuseAutodidact commented 8 years ago

using a postgres-specific data format in the db(Tournament model). this causes the test db, sqlite, to blow up

artasparks commented 8 years ago

LGTM. I plan on merging this later tonight or early tomorrow. SG?

brilee commented 8 years ago

In terms of API design, here are the conventions that people commonly use:

Create: POST /tournaments Get: GET /tournaments/<tournament_id> Update: PUT /tournaments/<tournament_id> Delete: DELETE /tournaments/<tournament_id>

In Create/Get/Update, the returned object should be the json body of the created/updated object, and in Create/Update, the submitted object should be the desired json body.

The HTTP request method is the verb, and the URL is the noun, so no need to have an extra /edit or /delete description on the URL.

artasparks commented 8 years ago

@brilee -- One question is: where do you think the URL for the form should go? for tournaments/new -- GET returns the form; POST creates a new tournament.

I suppose it could be GET/POST on /tournaments w/o the /new suffix.

brilee commented 8 years ago

Right, POST at the base URL. If there need to be multiple submission styles (form-encoded vs json), then the content-type header should be dispatched upon to decide how to interpret the submission.

artasparks commented 8 years ago

To clarify @brilee : my question was around returning the rendered HTML template. Where should such a URL live? Along side the POST?

I.e., what do you think about polymorphism for return values based on HTTP methods (GET/POST)?

brilee commented 8 years ago

There's another HTTP header called "Accept:" which should determine the return type - html or json. In practice, stuffing handlers for both HTML/JSON halves of the code into one method starts to get messy, so a lot of APIs use a different subdomain ("api.example.com") or subpath (example.com/api/...) to differentiate.

TheObtuseAutodidact commented 8 years ago

@brilee, Thanks! Super-useful.

artasparks commented 8 years ago

Seems like a reasonable approach to split based on an api path prefix - I've used this approach before in other web servers. If we do that, we should be consistent about that. I see there's already an api subdirectory with an api subpath, but some of the methods exist at the root that should be in API (I'm looking at you @ratings.route('/players', methods=['POST'])).

One small downside is that it makes it harder to make orthogonal components. Currently, the tournament exists in the web/app/tournament path. I suppose this would be split into web/app/api/tournament web/app/models/tournament web/app/models/tournament and then a views-target at the root level. This is very nearly what already exists. Another option is

web/app/tournament/api, web/app/tournament/models, etc.

Both would be fine, but I think it would make sense for a future PR. This PR has been going on for awhile anyway and it would be nice to get merged