AdventureLookup / adventurelookup-backend

Platform, Framework, Database software
GNU General Public License v3.0
28 stars 7 forks source link

Add endpoint for getting adventure information #23

Closed whonut closed 8 years ago

whonut commented 8 years ago

This addresses #19. The URL patterns are probably not what we want but I thought that was something best discussed. Any and all comments and criticisms are welcome.

codecov-io commented 8 years ago

Current coverage is 100%

Merging #23 into master will not change coverage

@@           master   #23   diff @@
===================================
  Files           6     8     +2   
  Lines          89   126    +37   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits           89   126    +37   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last updated by f882bd7...72ec194

probably-not-a-cat commented 8 years ago

I'm kind annoyed that those Django api tutorials just suggest to make serialized classes, when this is clearly better!

Just an fyi, one of your tests is failing, but not failing travis. Which also explains why codecov isn't 100%.

whonut commented 8 years ago

Which test? They all pass on my machine...

probably-not-a-cat commented 8 years ago

https://travis-ci.org/AdventureLookup/adventurelookup-backend/jobs/134558640#L528

probably-not-a-cat commented 8 years ago

@whonut I would suggest merging with master since bad tests should fail the travis build now.

Also looks like it can't connect to the website on travis. That'll be fun to debug ;)

pejter commented 8 years ago

You should also add newline at the end of each file to comply to PEP8.

whonut commented 8 years ago

@pejter Sublime Text displays a spurious blank line when you do that apparently! That'll explain why my linter was shouting at me for leaving (what I saw as) 2 blank lines at the end of a file. I've fiddled with my preferences, won't be problem now.

I'm feeling around blind trying to fix that failing test, everything works on my machine. Sorry if I do/did something stupid.

OK that 'fix' was (as I suspected) not one. I'm out of ideas.

probably-not-a-cat commented 8 years ago

@whonut I would recommend making a new branch when it comes to testing travis. That way you can keep this PR clean. You can rebase and merge back here later.

pejter commented 8 years ago

objects.create only returns the instance of the object. You need to call .save()on the instance for the change to be applied to the database. From Django docs: Note that instantiating a model in no way touches your database; for that, you need to save().

Some other points:

whonut commented 8 years ago

@probably-not-a-cat OK, sorry about that. Should I squash that last one? @pejter create saves it though...? Docs:

A convenience method for creating an object and saving it all in one step. Thus:

p = Person.objects.create(first_name="Bruce", last_name="Springsteen")

and:

p = Person(first_name="Bruce", last_name="Springsteen") p.save(force_insert=True)

are equivalent.

Thanks for the help with the tests. I think I've done it right now but I'll wait to hear back before pushing.

I can only apologise for my ineptitude with this. Thank you both.

probably-not-a-cat commented 8 years ago

I wouldn't. I tried squashing a PR before and github doesn't handle it nicely. It's not a big deal to leave it.

And no worries! Thanks for doing this!

probably-not-a-cat commented 8 years ago

Just an FYI, sometimes travis builds have issues that are not because of your code, which happens to be why it failed here. I would just wait a bit and restart the build.

whonut commented 8 years ago

Just FYI, I've made a class-based version, if that's what we're going with.

probably-not-a-cat commented 8 years ago

This looks good to me. 👍

probably-not-a-cat commented 8 years ago

Since there's no activity on here, and all concerns have been addressed, I would :shipit: (merge).

pejter commented 8 years ago

Well we did settle on using the class-based approach so I was waiting for that. @whonut has to merge that into this branch and we can merge it.

whonut commented 8 years ago

And I was waiting for the go-ahead 😝 Will merge.

whonut commented 8 years ago

I've merged the class-based branch now. It shows up in the commits tab, if not here.