[Moscow.pm] прошу помощи с ревью модуля

[PEF Secure]

On Saturday, November 29, 2014 15:03:44 Ilya Chesnokov wrote:
> > Если в этом смысле, то это не классический ORM, как я их видел. Классы
> > создаются динамически исходя из структуры базы в момент коннекта или при
> > первом обращении, если таблицу создали уже после того, как был коннект к
> > базе.
> 
> Ну вот это и надо бы написать в документации в начале раздела
> "DESCRIPTION".

Хорошо.
 
> Но к слову динамическое создание схемы в памяти есть и в DBIx::Class,
> но не рекомендуется из-за долгого времени загрузки в этом случае:
> https://metacpan.org/pod/DBIx::Class::Schema::Loader#make_schema_at
> https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookb
> ook.pod#Statically-Define-Your-Schema
 
Загрузка интерпретатора Perl уже не быстрый процесс, время на стартап, 
поэтому, считаю не особо критичным. Запущенное приложение имеет уже все классы 
в своём распоряжении, последующие форки уже имеют всю схему. Да и сам процесс, 
мне кажется, у меня легче и прямее организован, чем в DBIx::Class. По крайней 
мере, пока.

> > "Лёгкость" была главным аргументом. Легко начать использовать, создаются
> > "лёгкие" объекты. Простые вещи делаются просто -- на мой взгляд уже проще
> > некуда.
> 
> Ок. Думаю, тоже можно добавить в описание модуля.

Хорошо. Понимание что написать в документации для меня ценно.

> В принципе понятно, если задуматься, но не очевидно :) Так что не
> помешал бы комментарий или более привычный способ сообщения об ошибке.

Хорошо.

> Лучше было бы показать схему перед примерами, чтобы было понятнее.

Ридми написано "для затравки", а текст первого сообщения в этом списке 
рассылки ещё короче, чтобы быть прочитанным. В коротком сообщении трудно 
описать все тонкости, я подумаю над сообщением для следующкго раза.


> Но все же модуль этот - расширение над DBI, поэтому DBIx.
> Если бы он работал непосредственно с SQL, без соединения с БД, то был
> бы SQL:: (может я ошибаюсь - есть ли на CPAN подобные модули,
> называющиеся начиная с SQL::?).

Хорошо, будет DBIx::, осталось придумать второе слово. Light? ;)


> "Когда пишете код, представляйте, что следующий, кто будет его
> поддерживать - сумасшедший маньяк, который знает, где вы живете" :)

Я читал это :) Основной мой вопрос в уровне интеллекта этого маньяка. При 
некотром напряжении мысли, мой код понимабелен "средним по больнице" 
программистом. Это проверено на практике. Я, тем не менее, постараюсь 
реструктурировать код в более понятный и даже комментарии добавлю :)


> Комментарии нужно писать постоянно и менять с изменением кода. Или
> писать самодокументированный код: например, вместо
> $client->filter_timestamp писать
> $client->strip_microseconds_from_timestamp_columns (я не предлагаю это
> делать - только привожу пример, как это могло бы выглядеть более
> читабельно).

Да, читабельности такой вариант добавляет, но отнимает писабельность. Нужен 
компромисс. Изначально была мысль формат добавить, но оказалось не 
востребовано, везде всех устроил дефолтный формат без микросекунд. Я открыт 
для предложений и сам подумаю.

> > Он нужен, чтобы отработал Exporter.
> Это понятно, я скорее имел в виду, нужен ли там последний параметр -
> @_? Насколько я понял, вы из него уже вытащили все вам необходимое.

Не всё, в @EXPORT_OK есть один параметр, теоретически, можно придумать ещё 
какие-то. Я не совсем понял проблему. Разбор массива параметров импорта, на 
мой взгляд, не так уж страшен. Он решает проблему одиночных и парных 
параметров.

> Сам по себе механизм неплох, но не очень привычен для CPAN-модуля.

Согласен, я сделаю обёртку, которая будет передаваться при импорте.

> К слову о пространствах имен - вы генерируете классы DBC::* и DBQ::* -
> где гарантия, что в коде не будет уже классов с таким именем? Может
> стоит добавить имя базового модуля в качестве префикса - т.е. что-то
> типа SQL::Struct::DB[C|Q]::*?

Я не люблю длинные имена без необходимости. Я не люблю, когда для выражения 
мысли нужно написать много "букав". Я люблю перл за возможность выражения 
мысли кратко. Конкретно по этим префиксам я проверил CPAN на их отсутствие, но 
я признаю, что в глобалном смысле это надо конфигурировать. Я добавлю эту 
возможность. 

> Зато die() побыстрее.

"Быстрее", обычно, это когда умирать не надо :) Я не задумывался над аспектом 
скорости в сравнении croak и die, может быть действительно имеет смысл 
отказаться от Carp.

> Это безусловно "умно", а следовательно нарушает принцип KISS.

Объект коннектора там настраивается, поэтому он не может быть state. Условная 
проверка в том месте, что должно инициализировать только один раз, а затем 
безусловно возвращаться -- лично меня несколько "расстраивает", поэтому я 
применил фокус. На счёт понятности фокуса, тут сложно мне судить, но, вроде, 
ничего сложного.

> Шаблоны - это точно перебор, а heredoc - самое то. Плюс Sub::Quote.

Я подумаю при реструктуризации.


> Не стоит это воспринимать как приговор - это всего лишь первое ревью :)

Я так вообще рад, что нашёлся хоть кто-то, способный прочитать и грамотно 
разобрать мой код :) Что найдётся над чем работать я ожидал. 

> Отлично. Исправляйте основные проблемы и публикуйте.

Хорошо.

> По большому счету единственная серьезная проблема - это 30-секундное
> ожидание при отсутствии соединения с базой.

Для меня это было скорее решением, чем проблемой :) Вернее, это решение для 
относительно кратковременного пропадания коннекта к базе.

> Также нужно решить вопрос
> об имени модуля и пространстве имен генерируемых классов. Остальное в
> порядке "желательного". Даже error reporting можно оставить как есть -
> только описать его в документации.

Хорошо.

>  - везде 'message', а тут 'answer' - ошибка?

Да, ошибка, спасибо! :) Оно не было ошибкой до трансформации модуля.

> Для своего дистрибутива можно создать отдельный репо, как многие делают.

Для себя можно просто пользоваться модулем и никуда не выкладывать. 
-- 
PEF Developer