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

Add support for custom user models in Django 1.5+ #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maestrofjp
Copy link

Fixes #6. This is backwards compatible with non-custom user models as well.

Minor additions:

  • Fixed line length for pep with the normal standard being 120 (updated flake TravisCI config)
  • Cleaned up files to meet PEP8 (mostly stuff that flake isn't picky about)
  • Updated requirements-dev.txt to include requirements needs for proper module introspection / hinting while developing locally (i.e. get rid of PyCharm snags of missing packages)

…ort for django.contribu.auth.models.User for older applications. Fixes Kami#6
script:
- flake8 django_yubico/
script:
- flake8 django_yubico/ --max-line-length=119
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this option and stick with the default line length which is documented in pep8 (http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length)

Copy link
Owner

Choose a reason for hiding this comment

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

Besides that, other changes look good to me.

Copy link
Author

Choose a reason for hiding this comment

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

This package has some long lines and having them at 79 reduces readability considerably.

http://bugs.python.org/issue18472

This ticket is the conversation about recent changes to the wording in PEP 8 and even Guido says 100 characters is acceptable these days due to newer hardware / displays. Even the 79 character limit is no longer even followed by most devs in the standard Python library (and it's mentioned in the thread as well).

https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/

Django no longer limits to 79 either and I feel they state the reason beautifully:

One big exception to PEP 8 is our preference of longer line lengths. We’re well into the 21st Century, and we have high-resolution computer screens that can fit way more than 79 characters on a screen. Don’t limit lines of code to 79 characters if it means the code looks significantly uglier or is harder to read.

Instead of setting a hard line length, ,y suggestion instead of having flake warn about line lengths that we ignore E501 (the line length error code) just like Django does:

- flake8 django_yubico/ --ignore=E501

http://pep8.readthedocs.org/en/latest/intro.html#error-codes

Copy link
Author

Choose a reason for hiding this comment

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

The only place I know that really is set hard at 80 is Google but they stuff like 2 space indents as well. Even Java at Google is set at 100 and I know they are rallying to change that limit internally (hence the wording changes to PEP 8 last year saying it's ok).

Copy link
Author

Choose a reason for hiding this comment

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

The reason why I changed the line length is backends.py was throwing errors from flake (this is before I even submitted my PR).

Line 39-ish:

        try:
            if count > 1:
                # More then 1 OTP provided, using multi mode
                status = client.verify_multi(otp_list=otp,
                                             max_time_window=
                                             YUBICO_MULTI_TIMEOUT)

This area is was failing before the PR on line 43 with E251 (unexpected spaces around keyword). It doesn't like the carriage return after `max_time_window=``

Changed it to this:

        try:
            if count > 1:
                # More then 1 OTP provided, using multi mode
                status = client.verify_multi(otp_list=otp,
                                             max_time_window=YUBICO_MULTI_TIMEOUT)

Fixes the E251 error, but then I get a E501 (line too long -- 83 characters).

Changed it to this:

        try:
            if count > 1:
                # More then 1 OTP provided, using multi mode
                status = client.verify_multi(otp_list=otp,
                                         max_time_window=YUBICO_MULTI_TIMEOUT)

I can under indent the max_time_window line to fixe E251, but then flake gives me E128 (continuation line under-indented for visual indent).

Change it to this:

        try:
            if count > 1:
                # More then 1 OTP provided, using multi mode
                status = client.verify_multi(
                    otp_list=otp,
                    max_time_window=YUBICO_MULTI_TIMEOUT)

Finally, flake doesn't complain. That was a lot of effort to get it to work in flake and honestly it's kinda ugly.

Django project nicely ignores:

ignore=E123,E128,E265,E501,W601

Because in certain situations it makes sense to break those rules.

I'll make the change to the PR like you asked, but I think it would be nice if flake was a bit nicer to devs -- right now it's a bit overly eager IMHO.

Let me know what you'd like to do... I think adding in the list of ignores used by Django (and a bunch of other projects) is the most friendly and gives the decision to a dev to break one of those rules instead of sacrificing readability for the sake of a rule.

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.

Add support for Django custom user in contrib auth
2 participants