maccesch / cmsplugin-contact

Extendable contact plugin for django-cms with spam protection and i18n
BSD 2-Clause "Simplified" License
71 stars 61 forks source link

Pep8 #33

Closed Lacrymology closed 12 years ago

Lacrymology commented 12 years ago

Hello. I've customized a lot of things in my fork, and keeping up with your updates is very difficult.

This is a harmless and mostly cosmetic update, it basically just makes all the code pep8-compatible, at least in line lengths.

mitar commented 12 years ago

Personally, I am not so enthusiastic about short line lengths on modern computers. But I will pass this call ...

Lacrymology commented 12 years ago

well, I for one still sometimes code, or at least less through files, in consoles, that are not FORCEFULLY 80 chars wide, but which DO wrap-over once whichever width you made it is done, and it's nice to have a max width.

80 char is not worse than any other limit, and it being a standard, IMO is better than many

mitar commented 12 years ago

I am against pulling this in. Maybe wrapping is good, but not the simple/automatic way it is done here. Such things are simply not readable:

self.fields['recaptcha_'
            'response_field'] = self.fields.pop('recaptcha_'
                                                'response_field')
maccesch commented 12 years ago

But this is the only weird formatting. Everything else increases readability or at least doesn't deminish it. So we could merge it and fix this file.

And having a limited width is advantegous. Coding almost everything in Eclipse I often view files side by side so even my full hd display isn't enough to display too long lines.

mitar commented 12 years ago

I just do not like how it is wrapped. I find this way of wrapping, when you wrap from the end of the line, ugly. Because then it is too much code on the right.

Another example:

    akismet_api_key = KeyField(max_length=255,label=_("Akismet API Key"), 
                               help_text=_('Get a Wordpress Key from '
                                           'http://akismet.com/'))

I prefer:

    akismet_api_key = KeyField(
        max_length=255,
        label=_("Akismet API Key"), 
        help_text=_('Get a Wordpress Key from http://akismet.com/'),
    )
maccesch commented 12 years ago

Yes I prefer that too... So what do we do?

mitar commented 12 years ago

Close this pull request and leave to @Lacrymology or somebody else or us to rewrap it better.

I think the problem is that tools for automatic wrapping wraps in the non-preferred way. I do not know why. Maybe it is easier to implement?

maccesch commented 12 years ago

Hm in Eclipse it is configurable, but of course automatic wrapping always creates some weirdness. Ok then. I will open an issue.

Lacrymology commented 12 years ago

Just FYI, I didn't use automatic tools, I mean, my editor indents stuff when I make a newline, but I didn't run an "auto wrapper" script of any kind, I did it the way I thought it was less intrusive.

I'll look at these lines again, and fix them

Lacrymology commented 12 years ago

So just to be sure, you prefer

call_to_function(
    param1,
    param2,
    param3,
)

always to

call_to_function(param1, param2,
                       param3)

And what should I do with things like

self.fields['recaptcha_response_field'] = self.fields.pop('recaptcha_response_field')

where the left-hand elements are already very long? I could always just leave that one as a long line

maccesch commented 12 years ago

Thanks for your effort! Yes please do the first one.

As for the assignment you could leave it or do

self.fields['recaptcha_response_field'] = \
    self.fields.pop('recaptcha_response_field')
mitar commented 12 years ago

Sometimes something like this is useful:

self.fields.update({
    'recaptcha_response_field': self.fields.pop('recaptcha_response_field'),
})

BTW, what exactly does this line do? Set field back to itself?

Lacrymology commented 12 years ago

I think it makes sure it's the last field on the form

mitar commented 12 years ago

Aaa, because it is ordered dict. OK.