Skip to content

RESTful refactoring of /course access continued - #1542

Merged
dmitchell merged 1 commit into
masterfrom
dhm/restful_course
Oct 30, 2013
Merged

RESTful refactoring of /course access continued#1542
dmitchell merged 1 commit into
masterfrom
dhm/restful_course

Conversation

@dmitchell

Copy link
Copy Markdown
Contributor

Move index access into the url
Move course creation into the url
Add helper methods for testing to serialize json data and set accept header.

@cahrens this is part of my story but seemed big enuf to do as a PR, plz revie
@singingwolfboy plz review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this here and renamed from user.index()

Move index access into the url
Move course creation into the url
Add helper methods for testing to serialize json data and set accept header.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be clearer to return early, rather than set a variable and return it at the end of the function.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer single points of return (generally advised in "clean code"). And I wrote that code. :)

@dmitchell

Copy link
Copy Markdown
Contributor Author

@singingwolfboy @cahrens May I merge this? (the test failure is a false alarm on quality)

@cahrens

cahrens commented Oct 30, 2013

Copy link
Copy Markdown

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use Django's reverse function, rather than hard-coding the URL

@singingwolfboy

Copy link
Copy Markdown
Contributor

😩 👍 😩

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.

3 participants