Conversation
The serializer uses nested representations so that all details about the adventure can be retrieved with a single API call.
This will be neater as more tests are added.
Using nested serializers for related objects in AdventureSerializer and adding custom create and update methods allows all objects needed for an Adventure (Edition, Authors etc.) to be created from a single API call. This is as opposed to having endpoints for creating Authors, Settings etc.
The real work is done by the serializer. This just uses generic views.
|
Hey Dylan, Looks great to me - especially like the level of tests that you've got going here. Just a couple of comments: It looks like you've been applying flake8 as a linter to this, which is great, but the project was set up using pylint which complains a little bit about your code: Most of this is just simple docstrings and having run it over the rest of the codebase I can see that that is mainly because the default Now I would like to have it so that the CI runs pylint over the code before passing the tests (I might turn that on now). This .pylintrc came from a previous project and I'm mostly happy with it. However, if you think that some of the rules it has aren't any good or if you think that pylint is the devil and flake8 is the only way forward, then I'm happy to change this up. I would like all code to conform to some linting standard though (we have eslint for the front end), so we somehow have to make all of this pass! Either that means adjusting this commit to fit the pylintrc, or changing the pylintrc, or changing to flake8 to make it all work. Let me know what you think! Finally, we'll need a Just want to say that I really appreciate the work you've put into this so far and I think what you've produced is excellent. I know you may be short of time in the near future so if you don't think you can devote the time to fixing these minor issues then I'm happy to take it on myself - just let me know. Cheers, Peter |
Typos in AdventureSerializer.update meant that PUT and PATCH requests were overwriting instance.title with a max_level value.
|
FYI I've updated the master branch so that it passes lint tests - you will have to merge the updated master back in with your code before recommitting. Sorry! |
|
Does this want closing? |
2 participants
Still have some stuff to add. Just getting this up for feedback sooner rather than later.