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

PEP8 #39

Closed
ernado opened this issue Dec 15, 2013 · 15 comments
Closed

PEP8 #39

ernado opened this issue Dec 15, 2013 · 15 comments

Comments

@ernado
Copy link

ernado commented Dec 15, 2013

W191 indentation contains tabs
E501 line too long (X > 79 characters)
E223 tab before operator

Примутся ли чисто рефакторинговые пулл-реквесты, чтобы код больше соответствовал стандартам?
Открыв код в pycharm, мой монитор стал настолько желтым, что у меня в комнате поднялась температура :)

Конкретнее:
Заменить все табы на пробелы из 4 символов
Сократить длину строк до 79 символов
More and more

@ernado
Copy link
Author

ernado commented Dec 16, 2013

И вообще хотелось бы больше комментариев в коде, хотя бы для функций/методов, потому что не всегда с наскоку понятно, что вообще происходит.

@ernado
Copy link
Author

ernado commented Dec 16, 2013

По поводу именования переменных:
http://www.python.org/dev/peps/pep-0008/#naming-conventions

Рекомендуется их делать не кемелом, а lower-case с подчеркиваниями.
Т.к. кодовая база сейчас не огромная, можно исправить, пока не поздно

@mrDoctorWho
Copy link
Owner

@ernado,
W191 indentation contains tabs — я категорически против идентации пробелами
E501 line too long (X > 79 characters) — хоть и прошли те времена, когда экран ограничивался 80-ю символами в строку, я стараюсь ограничивать длину там, где это представляется возможным.
E223 tab before operator — это вообще где? В gateway.py ничего такого нет.
Пожалуй, я приму пулл-реквесты, исправляющие: E302, E251, E501 (без нарушения внешнего вида кода), E201, E101 (если такое есть вне задания dict и list), E262.

По поводу именования переменных: меня устраивает текущий вид. Задумано так: константы, задаваемые в конфиге (USER_LIMIT, THREADING_STACK_SIZE, и т.д.) должны быть upper-case с подчеркиванием, а остальные в UpperCamelCase.

Насчёт комментариев: не думал, что это кому-то интересно. Буду комментировать.

@ernado
Copy link
Author

ernado commented Dec 16, 2013

@mrDoctorWho
Отвечаю по порядку
W191 wontfix, попробую настроить среду разработки, чтобы не текли кровавые слелы в таком случае.
E223 - я выполнял pep8 *
Кстати говоря, вам не кажется, что gateway.py слишком большой?

Константы, ясное дело, должны быть upper case, но не очень ясен выбор UpperCamelCase. Он же уже для имен классов используется, возникает неодозначность.

W601 .has_key() is deprecated, use ‘in’ тоже можно исправлять?

В итоге:
исправляю все E_, игнорирую W1_

@ernado ernado mentioned this issue Dec 16, 2013
@mrDoctorWho
Copy link
Owner

@ernado,

gateway.py слишком большой?

Он был больше, я переместил обработчики событий в «handlers/». Насчёт размера я согласен, но ничего не поделать. Если у вас есть какие мысли — готов их выслушать.

не очень ясен выбор UpperCamelCase

Дело вкуса, мне лично нравится. Можете кинуть в меня камень, но я согласен не со всеми пунктами PEP8. Для меня важна красота и оптимизация кода (пусть даже кое-где с этим не очень).

W601

Да, конечно, исправляйте.

@mrDoctorWho
Copy link
Owner

  • за исключением E401

@ernado
Copy link
Author

ernado commented Dec 16, 2013

Благодаря игнорированию E401 у вас куча ненужных импортов и их неоднородность.
Рекомендуется сначала импортировать встроенные модули, затем сторониие, затем внутренние.
Но это еще нормально.

У вас в gateway.py вообще ничего не импортируется из внутренних модулей, а используется хак с окружением.
Собственно, extensions, handlers и library не являются модулями, а просто папки с кодом.
С таким же успехом можно использовать для всего проекта один файл, или одну директорию с кучей файлов, игнорируя за одно вообще всё. Это лишь иллюзия структуры.

Вы импортируете logger, а в gateway почему-то логируете через print. Используете threading, но не хотите переходить на python3 или хотя бы поддерживать его, хотя в нем улучшен GIL.

Пожалуй, я пока не буду трогать gateway, а начну понемногу проходиться по небольшим внутренним подулям, стараясь не сломать обратную совместимость.

Кстати, если уж в проекте используется не-pep8 стиль кода, пожалуйста, оформите его в гайдлайн, чтобы я мог оформлять код по вашим правилам.

@ernado
Copy link
Author

ernado commented Dec 16, 2013

По поводу camelcase - в коде испольуются стандартные модули python, а они почти все написаны с соблюдением pep8. Получается смешивание стилей именования.

В проекте полностью нарушена структура кода.

@mrDoctorWho
Copy link
Owner

Благодаря игнорированию E401 у вас куча ненужных импортов и их неоднородность.

дело не в E401, а в том, что я выпиливал часть кода, но забыл убрать модули.

Рекомендуется сначала импортировать встроенные модули,

Разве у меня не так?

Это лишь иллюзия структуры.

В общем-то да. А у вас есть какие-то идеи по этому поводу?

Вы импортируете logger, а в gateway почему-то логируете через print.

Где?

Кстати, если уж в проекте используется не-pep8 стиль кода, пожалуйста, оформите его в гайдлайн, чтобы я мог оформлять код по вашим правилам.

Боюсь мне не удастся стандартизовать свой код. Для начала придётся привести весь gateway.py в один стиль.

@ernado
Copy link
Author

ernado commented Dec 17, 2013

дело не в E401

Это скорее косвенная причина. Когда импорты каждый на новой строчке, легче найти лишний, чем в простом списке. Да и убрать лишний можно просто удалением строчки. Минусы, вроде увеличения количества кода есть.
Удобней импортировать всё списком, но визуально потом работать проще, если по pep8.

Разве у меня не так?

Редко, но не так. Проблема в другом. Например, в gateway импорт-секция вообще смешана с кодом, хотя порядок импорта практически соблюден

Кстати, для удобного цитирования можете применять

> текст цитаты

ответ
ответ

> текст цитаты 2
ответ

Чтобы разметка не ломалась, нужны пропуски строчек.

Боюсь мне не удастся стандартизовать свой код

Пока что не очень очевидно лишь использование UpperCamel для констант, остальное более-менее понятно.

Вы импортируете logger, а в gateway почему-то логируете через print.
Где?

    print "#! Incorrect launch!"
    # хотя такого мало. Это бы лучше в logger.critical("Incorrect launch") ?
...
try:
    execfile(Config)
    Print("#-# Config loaded successfully.")
except:
    Print("#-# Config file doesn't exists.")
    exit()
# и прочие вызовы Print(),

Которая, насколько я понимаю, пишет не в логгер а в stdout, еще почему-то в UpperCamel и не понятно, чем отличается от print по названию

По поводу структуры

Простро приведу несколько примеров того, почему структуры как таковой нет:

sys.path.insert(0, "library")
reload(sys).setdefaultencoding("utf-8")
# ...
from itypes import Database
from webtools import *
from writer import *
from stext import *
from stext import _

# в extensions:
require("attachments")
# 

Как сделать очевиднее:
импортировать всё как

from vk4xmpp.library.itypes import *

(хотя это и не очень хорошо)
либо

from library.itypes import *

Но все равно придется что-то делать с pythonpath.

По поводу имен для модулей

IQ.py, vkApi.py и writer.py, например, испольуют сразу три вида именования.
Предложение привести всё к lowercase без подчеркиваний.

@ernado
Copy link
Author

ernado commented Dec 17, 2013

Исправление E128 с помощью табов невозможно, придется игнорировать

Какой смысл использовать табы, если вы выравниваете списки пробелами?

@mrDoctorWho
Copy link
Owner

    print "#! Incorrect launch!"
    # хотя такого мало. Это бы лучше в logger.critical("Incorrect launch") ?

нет, не лучше. Очень маловероятно, что на данном этапе кто-то полезет в лог.

@aawray
Copy link

aawray commented Dec 18, 2013

Дичайше поддерживаю затею с пробельной индентацией.

@ernado
Copy link
Author

ernado commented Dec 19, 2013

@mrDoctorWho
Логгер можно настроить для вывода в консоль. Так будет лучше.

@aawray
В проекте она смешанная, ведь списки (и не только они) все равно выравниваются пробелами и это превратится в треш при изменении отображаемого количества пробелов для таба.

@mrDoctorWho
Copy link
Owner

В пользу табов согласен выпилить выравнивание пробелами.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants