search

petertrotman / adventurelookup

Open repository for the website https://adventurelookup.com/
MIT License
5 stars 3 forks source link

Add models #9

Closed whonut closed 8 years ago

whonut commented 8 years ago

Addresses #5.

This is only the very beginnings, there will be more commits, but I thought that it would be best to open a PR straight away for discussion.

Monsters and items should be fairly straightforward, but it's not clear to me how to structure more freeform things like NPCs in a useful way. If we truly want people to be able to say 'I want an adventure with wizards in it', how are we going to let the system know who the wizards are? The only thing I can think of is a freeform description field.

petertrotman commented 8 years ago

Sweet! Looks like a great start to me, especially like the help text additions :-).

FYI, Semaphore does a migrate but it doesn't makemigrations first, so in order for the tests to pass you'll need to include the new migrations with the commit (should be as easy as python manage.py makemigrations then re-committing).

Also it would be nice to make sure we have a basic suite of CRUD tests for the models before we merge. Do you have time to write them? If not I'll do it.

whonut commented 8 years ago

Before I go about doing it for every model, is that last commit what you had in mind for tests?

petertrotman commented 8 years ago

Hey nice timing. Yeah those look great - although what you have in setUp is actually part of the test (so it should be in a test method); we just want to make sure that we can create the model with the keyword arguments that we expect.

Having thought about it, it's probably fine to just have a single test per model on create, since that accomplishes the same goal. So in my mind, each model test case should have a single method like test_create_model - instead of full CRUD tests. What do you think?

whonut commented 8 years ago

To clarify, you mean just one test for creation, so no tests for updating and deletion? If the purpose is just to make sure that all the arguments line up then yeah, that'd do it.

petertrotman commented 8 years ago

Great stuff, thanks whonut. Happy to merge this in now, unless you have more stuff you'd like to put onto it?

whonut commented 8 years ago

I'm happy. Obviously it's not complete but I think it's enough to be going on with.