Skip to content

Conversation

@MiguelSR
Copy link
Contributor

@MiguelSR MiguelSR commented Apr 1, 2016

No description provided.

binding = BINDING_HTTP_REDIRECT
if hasattr(conf, '_sp_authn_requests_signed') and \
conf._sp_authn_requests_signed:
binding = BINDING_HTTP_POST
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. But wouldn't a oneliner like this

binding = BINDING_HTTP_POST if getattr(conf, '_sp_authn_requests_signed', False) else BINDING_HTTP_REDIRECT

be more DRY and Pythonic?

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 agree with getattr over hasattr, thanks!

I'd prefer a nested if over a one-liner in this case, as I try to keep my lines under 80 to fit PEP8. Anyway since this file has other lines with this style I have no problem of re-writing it as you said.

I'll commit right now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This 80-chars per line is actually the only PEP8 rule that is officially not respected by Django guidelines, and also by most of other serious projects. The reasoning is quite obvious - this rule comes from the previous century and sounds ridiculous in today's era of huge and wide screens. From what I have observed, 120 columns seems to be more preferred these days, even though there is not a strict limit.

Now, back to the topic :-) Thank for the new commit, and if I can bother you just a bit more - would you mind squashing those two commits into one? Thanks :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! I've squashed them into a single commit, hope everything's fine.

Thanks,
Miguel.

@knaperek knaperek merged commit 4e940e8 into IdentityPython:master Apr 4, 2016
@knaperek
Copy link
Collaborator

knaperek commented Apr 4, 2016

Thank you!

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.

2 participants