Skip to content

Issue 410 #411

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

Merged
merged 6 commits into from
Feb 18, 2018
Merged

Issue 410 #411

merged 6 commits into from
Feb 18, 2018

Conversation

n2ygk
Copy link
Contributor

@n2ygk n2ygk commented Feb 7, 2018

I was a bit confused by the various sources of urlpatterns, etc. and hopefully got it right. I ran both the example app with runserver and did a tox run too to make sure I didn't break that.

For whatever reason, I was unable to get this to work by doing export DJANGO_SETTINGS_MODULE=example.settings and various django-admin.py stuff as I got an error about module example not found. I'm not quite sure why that happened.

However, doing basically the same thing by creating manage.py works and, since manage.py is the usual documented approach, I think that's OK.

It's a little different from the usual pattern in that there's not a project directory with settings and then an example app directory; both are the same, but that's OK I think.

There's still one problem in here in that http://127.0.0.1:8000/entries/2/relationships/suggested returns a 404. I must have missed something. @mblayman Maybe you can find it quicker than I can.

@n2ygk
Copy link
Contributor Author

n2ygk commented Feb 7, 2018

Ugh, looks like my edit to example/urls.py messed up.

n2ygk added 2 commits February 9, 2018 15:49
…orted properly.

- travis caught it. This time I've done the isort locally to confirm the order.
- MIDDLEWARE_CLASSES was replaced by MIDDLEWARE as of Django 1.10 and, according to
  tox.ini, this is only testing >=1.11.
@codecov-io
Copy link

codecov-io commented Feb 9, 2018

Codecov Report

Merging #411 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   91.79%   91.75%   -0.04%     
==========================================
  Files          55       55              
  Lines        2925     2923       -2     
==========================================
- Hits         2685     2682       -3     
- Misses        240      241       +1
Impacted Files Coverage Δ
example/settings/dev.py 100% <100%> (ø) ⬆️
rest_framework_json_api/renderers.py 85.32% <0%> (-0.49%) ⬇️
rest_framework_json_api/exceptions.py 84.61% <0%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5d34e2...926988a. Read the comment docs.

@n2ygk
Copy link
Contributor Author

n2ygk commented Feb 9, 2018

@mblayman Looks like now it's safe to merge. I had a bit of a learning curve with tox, travis, etc. manage.py test still fails though the travis and tox tests don't catch it.

Copy link
Collaborator

@mblayman mblayman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good. I'd like to understand how the DJA package is made available to example app. Did you happen to run the steps you listed in Getting Started from a totally fresh clone to see if it would start the example app properly?

Once I have a bit more confidence that these steps will install DJA for the example app, I'm happy to merge.

@@ -70,13 +70,17 @@ From Source

git clone https://github.com/django-json-api/django-rest-framework-json-api.git
cd django-rest-framework-json-api
pip install -e .
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work? I don't understand where the DJA package would be available to Python if it's not installed in editable mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's available because you are in the source tree for DJA so import rest_framework_json_api just works. I suppose retaining pip install -e . can't hurt. Pushing a new commit.

django-rest-framework-json-api$ tree -L 1
.
├── AUTHORS
├── CHANGELOG.md
├── LICENSE
├── MANIFEST.in
├── README.rst
├── docs
├── drf_example
├── env
├── example
├── manage.py
├── pytest.ini
├── requirements-development.txt
├── requirements.txt
├── rest_framework_json_api
├── setup.cfg
├── setup.py
└── tox.ini

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, neglected to answer the question: "Did you happen to run the steps you listed in Getting Started from a totally fresh clone to see if it would start the example app properly?"

Yes. Several times.

@mblayman
Copy link
Collaborator

Thanks! I appreciate all your work on this branch. I forgot to ask you to add your name to the AUTHORS file. We do that to give you credit for your contribution and recognize that you have some copyright claim to this project. The list is alphabetical by first name in case that's somehow unclear.

Copy link
Collaborator

@mblayman mblayman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @n2ygk!

@mblayman mblayman merged commit e501b04 into django-json-api:master Feb 18, 2018
@n2ygk n2ygk mentioned this pull request Aug 10, 2018
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.

3 participants