Skip to content
This repository has been archived by the owner on Nov 8, 2023. It is now read-only.

Refactor #3

Merged
merged 18 commits into from Jun 22, 2017
Merged

Refactor #3

merged 18 commits into from Jun 22, 2017

Conversation

unmade
Copy link
Contributor

@unmade unmade commented Jun 8, 2017 •
edited

Hello, Josh!

Thank you for a great app.

Unfortunately, I had have some troubles:

  • I've installed latest available version (v1.3) and it was missing static and templates files, so I have to copy it myself to make things work.
  • there were also errors in my browser about wrong content type for templates (application/json instead of application/javascript).
  • it turns out, that locking.urls could only be included with pattern ^locking/.

I fixed troubles above and also added localization (with russian locale available). I've also moved out tests from app folder.

FYI: I ran tox locally and everything went fine, except py34.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9379481 on unmade:refactor into ** on joshmaker:master**.

4 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9379481 on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9379481 on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9379481 on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9379481 on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

coveralls commented Jun 9, 2017 •
edited

Coverage Status

Changes Unknown when pulling a7b48ba on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

coveralls commented Jun 9, 2017 •
edited

Coverage Status

Changes Unknown when pulling 7dec646 on unmade:refactor into ** on joshmaker:master**.

@unmade
Copy link
Contributor Author

unmade commented Jun 9, 2017

Finally, all tests seems to pass successfully.
It turns out selenium==3.4.3 with py34 resulted in error

OSError: [Errno 9] Bad file descriptor

So I've fixed tox file and hardcoded selenium==2.53.6 version for py34.

Initially py35 was added to travis.yml, but it misses python3.5 interpreter:

ERROR: InterpreterNotFound: python3.5
___________________________________ summary ____________________________________
ERROR:   py35-django18: InterpreterNotFound: python3.5

@coveralls
Copy link

coveralls commented Jun 9, 2017 •
edited

Coverage Status

Changes Unknown when pulling 25a06a9 on unmade:refactor into ** on joshmaker:master**.

3 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 25a06a9 on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 25a06a9 on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 25a06a9 on unmade:refactor into ** on joshmaker:master**.

@joshmaker
Copy link
Owner

@unmade thanks for the pull request! There are a lot of commits to look through, so it may take me a little while before I can review them all.

Which browser is having problems loading content with the application/json content type? I ask because it looks like that is the correct content type as defined by the RFC 4627 spec. Maybe there is another way to work around the issue you are running into?

joshmaker
joshmaker suggested changes Jun 9, 2017
View reviewed changes
Copy link
Owner

@joshmaker joshmaker left a comment

Choose a reason for hiding this comment

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

It looks like you put a lot of work into this and most of the changes you have here look very good. I've left a few questions, but hopefully we can work together to get your fixes merged into my master branch soon.

.gitignore Outdated
@@ -1,7 +1,16 @@
*.pyc
__pycache__

.idea
Copy link
Owner

Choose a reason for hiding this comment

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

Minor issue, but I'd like to leave IDE files to the user's global gitignore file and only include build files specific to this project here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

CHANGELOG.rst Outdated
@@ -1,6 +1,14 @@
Changelog
=========

**1.3.1 (June 8, 2017)**
Copy link
Owner

Choose a reason for hiding this comment

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

Because this change includes both bug fixes and fixes, I think it warrants a bump to version 1.4. I'm going to try to follow the semantic versioning rules for these version numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to 1.4

locking/urls.py Outdated
@@ -6,6 +6,8 @@

__all__ = ('urlpatterns', )

app_name = 'locking'
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't look like this variable is used in this file, can we delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I follow this example from documentation https://docs.djangoproject.com/en/1.9/topics/http/urls/#id4

For example, specifying app_name in url conf gives you ability to reverse urls in package like reverse(locking:locking-api)

and include package urls in project like:

url(r'^locking/', include('locking.urls', namespace='lock')),

and reverse it reverse('lock:locking-api').

But, unfortunately, this doesn't make sense in Django 1.8

tests/settings.py Outdated
'django.template.loaders.filesystem.Loader',
'django.template.loaders.app_directories.Loader',
)
if django.VERSION < (1, 8):
Copy link
Owner

Choose a reason for hiding this comment

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

Versions less than 1.8 are no longer supported, so I think we can just delete this block of code instead of wrapping it in a conditional. (I guess we'd need to also remove Django 1.7 from the tox tests as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dropped support for Django 1.7

locking/admin.py Outdated
if object_id is not None:
reverse_kwargs['object_id'] = object_id

return reverse('locking:locking-api', kwargs=reverse_kwargs)
Copy link
Owner

Choose a reason for hiding this comment

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

This commit introduces a new required namespace for the locking API URLs. If users follow the instructions in the readme and add:

url(r'^locking/', include('locking.urls')),

To their urls.py file, then they will get the error NoReverseMatch: u'locking' is not a registered namespace. Is there a way to update locking/urls.py to add the namespace automatically?

If not, you'll need to:

  • update the readme
  • document this breaking change in the changelog
  • change the version number to 2.0.0 to reflect the fact that this has backwards incompatible changes

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, you're right, Django 1.8 will get NoReverseMatch error, if they don't provide proper namespace. For more info, see my comment for app_name.

I discarded my changes for urls and namespace for now.

locking/admin.py
})

def locking_admin_form_js(self, request, object_id):
"""Render out JS code for locking a form for a given object_id on this admin"""
return render(request,
'locking/admin_form.js',
{'options': self.get_json_options(request, object_id)},
content_type="application/json")
content_type="application/javascript")
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to try to stick to application/json because that's what the JSON spec recommends and that's what Django's built in JsonResponse object uses.

I'm using the JsonResponse object elsewhere in this project, so if that isn't causing your problems maybe there is a way to work around this issue? Let me know what problem you are seeing and maybe I can help.

Copy link
Contributor Author

@unmade unmade Jun 13, 2017 •
edited

Choose a reason for hiding this comment

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

In my console I see the following error: http://take.ms/2YO6t
My browser is Chrome 58.0.3029.110 (64-bit) on macOS Sierra

In Safari everything seems to be OK.

Anyway, wouldn't application/javascript right content_type for js file?

Copy link
Owner

Choose a reason for hiding this comment

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

Anyway, wouldn't application/javascript right content_type for js file?

Yep, it totally would. My mistake for thinking it was a .json file

@coveralls
Copy link

coveralls commented Jun 13, 2017 •
edited

Coverage Status

Changes Unknown when pulling 0811fcc on unmade:refactor into ** on joshmaker:master**.

@unmade
Copy link
Contributor Author

unmade commented Jun 13, 2017

@joshmaker Thank you for taking your time! I've made fixes to my PR and leave comments to make things more clear.

@coveralls
Copy link

coveralls commented Jun 13, 2017 •
edited

Coverage Status

Changes Unknown when pulling 6728c5f on unmade:refactor into ** on joshmaker:master**.

@coveralls
Copy link

coveralls commented Jun 13, 2017 •
edited

Coverage Status

Changes Unknown when pulling 082144e on unmade:refactor into ** on joshmaker:master**.

@unmade
Copy link
Contributor Author

unmade commented Jun 21, 2017

Josh, once again, big thanks to you! :)
You approved PR about a week ago. Are you gonna to merge it? I don't have write access to your repo.
Please, let me know, in case there is additional work to do

@joshmaker joshmaker merged commit fad0996 into joshmaker:master Jun 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joshmaker joshmaker joshmaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@unmade @coveralls @joshmaker