petertrotman / adventurelookup

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

Add basic api #10

Open whonut opened 8 years ago

whonut commented 8 years ago

Still have some stuff to add. Just getting this up for feedback sooner rather than later.

petertrotman commented 8 years ago

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:

$ pylint --rcfile .pylintrc --ignore migrations adventures
************* Module adventures.apps
C:  1, 0: Missing module docstring (missing-docstring)
C:  4, 0: Missing class docstring (missing-docstring)
************* Module adventures.admin
C:  1, 0: Missing module docstring (missing-docstring)
W:  1, 0: Unused admin imported from django.contrib (unused-import)
************* Module adventures.urls
C:  1, 0: Missing module docstring (missing-docstring)
************* Module adventures.models
C:  1, 0: Missing module docstring (missing-docstring)
C:  4, 0: Missing class docstring (missing-docstring)
C: 11, 0: Missing class docstring (missing-docstring)
C: 18, 0: Missing class docstring (missing-docstring)
C: 25, 0: Missing class docstring (missing-docstring)
C: 32, 0: Missing class docstring (missing-docstring)
************* Module adventures.serializers
C:  1, 0: Missing module docstring (missing-docstring)
C:  5, 0: Missing class docstring (missing-docstring)
C: 10, 0: Missing class docstring (missing-docstring)
C: 15, 0: Missing class docstring (missing-docstring)
C: 20, 0: Missing class docstring (missing-docstring)
C: 25, 0: Missing class docstring (missing-docstring)
************* Module adventures.views
C:  1, 0: Missing module docstring (missing-docstring)

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 django-admin apps don't provide them by default - I'll be sure to add them in myself.

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 RetrieveUpdateDestroyAPIView for the adventures details page, and we'll need to provide appropriate permissions to the views (i.e. staff can do everything, registered users can create and edit their own adventures (maybe destroy?), and anyone can retrieve).

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

petertrotman commented 8 years ago

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!

whonut commented 7 years ago

Does this want closing?