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

APD-246 - [BE] migrate project to work under Django master branch #13

Merged
merged 8 commits into from Apr 19, 2017

Conversation

poxip
Copy link
Contributor

@poxip poxip commented Feb 9, 2017

This also provides backwards compatibility with Django 1.8, 1.9, 1.10, 1.11a1

This also provides backwards compatibility with Django 1.8, 1.9, 1.10, 1.11a1
@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage increased (+0.8%) to 47.554% when pulling d215f7f on feature/APD-246-migrate-to-django-master into 4c77aea on master.

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage increased (+0.8%) to 47.554% when pulling 3c0f780 on feature/APD-246-migrate-to-django-master into 4c77aea on master.

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage increased (+0.8%) to 47.554% when pulling 3c0f780 on feature/APD-246-migrate-to-django-master into 4c77aea on master.

@jacoor jacoor requested review from remik and jacoor February 20, 2017 12:40
@@ -2,7 +2,11 @@
from django.views.generic import TemplateView
from django.conf import settings
from django.contrib.admin.views.decorators import staff_member_required
from django.core.urlresolvers import reverse
try:
from django.urls import reverse
Copy link
Contributor

Choose a reason for hiding this comment

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

Recall my comment in Demo project? This one should be done as file and imported from there, even in demo.

@@ -1,3 +1,2 @@
Django>=1.8.7, <1.9 # rq.filter: >=1.8.7,< 1.9
Copy link
Contributor

Choose a reason for hiding this comment

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

No Django at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good one. Probably pip complains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is because pip complains while running tests under tox.

'ydcommon.views.qunit_view',
name='qunit'),
)
urlpatterns = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to PR but @jacoor are we using it all? Maybe time for cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Until we decide to use some FE CMS this has to stay.

tox.ini Outdated
py34-django18,
py35-django18,
py36-django18,
{py27,py34,py35}-django18
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge all (py 27-35) to 1 line.
py36 separate.

tox.ini Outdated
py34: python3.4
py35: python3.5
py36: python3.6
py27: python2.7
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you sort it?

tox.ini Outdated
{py27,py34,py35}-django19
{py27,py34,py35,py36}-django110
# add django1.11 after release
{py34,py35,py36}-django{master}

[testenv]
Copy link
Contributor

Choose a reason for hiding this comment

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

Add option for master to allow fail.

dest='xml_output',
help='Render as XML'),
)
class Command(BaseCommand):
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test it? This command is not working since Django 1.8.

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed by @poxip

@@ -1,7 +1,6 @@
import sys
Copy link
Contributor

Choose a reason for hiding this comment

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

@jacoor do we use it at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Until we decide to use some FE CMS this has to stay.

@@ -3,11 +3,10 @@
import re
Copy link
Contributor

Choose a reason for hiding this comment

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

@jacoor do we use it at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Until we decide to use some FE CMS this has to stay.

@@ -13,7 +17,7 @@ class QunitTestsView(TemplateView):
def get_context_data(self, **kwargs):
context = super(QunitTestsView, self).get_context_data(**kwargs)
if not kwargs['path'] or kwargs['path'] == 'index':
for template_dir in settings.TEMPLATE_DIRS:
for template_dir in settings.TEMPLATES[0]['DIRS']:
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to check both versions.

.travis.yml Outdated
@@ -7,6 +7,8 @@ python:

env:
- DJANGO="django>=1.8,<1.9"
- DJANGO="django>=1.9,<1.10"
- DJANGO="django>=1.10,<1.11"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing master version.
Maybe convert it to tox-travis version eg. https://github.com/ArabellaTech/drf_tweaks/blob/master/.travis.yml

@coveralls
Copy link

coveralls commented Feb 26, 2017

Coverage Status

Coverage decreased (-4.9%) to 41.84% when pulling 98bfa7c on feature/APD-246-migrate-to-django-master into 4c77aea on master.

@coveralls
Copy link

coveralls commented Feb 26, 2017

Coverage Status

Coverage increased (+1.04%) to 47.758% when pulling ea33777 on feature/APD-246-migrate-to-django-master into 4c77aea on master.

data = RequestContext(request)
for template_dir in settings.TEMPLATE_DIRS:
data = RequestContext(request).dicts[0]
for template_dir in settings.TEMPLATES[0]['DIRS']:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for new Django, we should support both versions.

@@ -13,7 +13,11 @@ class QunitTestsView(TemplateView):
def get_context_data(self, **kwargs):
context = super(QunitTestsView, self).get_context_data(**kwargs)
if not kwargs['path'] or kwargs['path'] == 'index':
for template_dir in settings.TEMPLATE_DIRS:
template_dirs = (
list(getattr(settings, 'TEMPLATES', [{'DIRS': []}])[0]['DIRS']) +
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it not break for old way? There won't be 0 key.

@coveralls
Copy link

coveralls commented Mar 1, 2017

Coverage Status

Coverage increased (+1.1%) to 47.852% when pulling 8a6f41e on feature/APD-246-migrate-to-django-master into 4c77aea on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 47.852% when pulling 8a6f41e on feature/APD-246-migrate-to-django-master into 4c77aea on master.

@remik
Copy link
Contributor

remik commented Mar 1, 2017

@poxip Still missing changes for templates dir.

@poxip
Copy link
Contributor Author

poxip commented Mar 2, 2017

@remik Yes, I know that, I just didn't have the time yesterday to get everything done (doing it today tho) ;)

@coveralls
Copy link

coveralls commented Mar 2, 2017

Coverage Status

Coverage increased (+2.7%) to 49.432% when pulling a6141c1 on feature/APD-246-migrate-to-django-master into 4c77aea on master.

@poxip poxip merged commit db53047 into master Apr 19, 2017
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.

None yet

4 participants