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 models #9

Merged
merged 11 commits into from
Sep 9, 2016
Merged

Add models #9

merged 11 commits into from
Sep 9, 2016

Conversation

whonut
Copy link
Contributor

@whonut whonut commented Sep 9, 2016

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.

The Adventure model is currently very minimal. It does
not track things like the items, monsters or NPCs in
an adventure.
@petertrotman
Copy link
Owner

petertrotman commented Sep 9, 2016

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
Copy link
Contributor Author

whonut commented Sep 9, 2016

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

@petertrotman
Copy link
Owner

petertrotman commented Sep 9, 2016

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
Copy link
Contributor Author

whonut commented Sep 9, 2016

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.

The only purpose of testing the models is to make sure that we can
use the expected keyword arguments during object creation. For this
purpose, full CRUD testing is not necessary. Testing object creation
is enough.
@petertrotman
Copy link
Owner

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

@whonut
Copy link
Contributor Author

whonut commented Sep 9, 2016

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

@petertrotman petertrotman merged commit 59104e2 into petertrotman:master Sep 9, 2016
@whonut whonut deleted the add-models branch September 10, 2016 14:53
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.

2 participants