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 adventure editing #25

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

Conversation

whonut
Copy link
Contributor

@whonut whonut commented Jun 13, 2016

Partially addresses #20. I expect this to need some work but I thought I'd get the ball rolling.

The commit history is a little bit wonky because I had a bit of a disaster locally and I'm too scared of losing my code again to try and make it look pretty. I hope that won't cause any issues.

Tests for put requests in AdventureById currently fail.
The AdventureById view remains unchanged, so the put method tests
currently fail.
When accessing the Adventure object directly, we have no QuerySet and
so cannot call update(), instead we have to call save(). Storing the
adventure ID means we can access the Adventure via filter(). This
returns a QuerySet, so we can call update().

Calling update() instead of save() is more efficient and avoids a race
condition wherein an object is changed in the database between
retrieval and saving.
No attempt is made at authentication, nor at proper CSRF-handling.
The assumption is made that authentication will be handled by a
parent view.
Local repo was screwed up. This brings the local and remote
repos to the same point.
@pejter
Copy link
Contributor

pejter commented Jun 13, 2016

I yapf working correctly? Seems like the line length is not set correctly. Besides that I believe we're good to go unless someone has anything to say.

@whonut
Copy link
Contributor Author

whonut commented Jun 13, 2016

Dang it. I took it out of the pre-commit hook because it was styling files I didn't want it to and I got annoyed by all the git status and git diff spam. Stand by.

@whonut
Copy link
Contributor Author

whonut commented Jun 13, 2016

Turns out you're not meant to use hooks for that. Live and learn.

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.

None yet

2 participants