29 ноября 2014 г., 12:03 пользователь PEF Secure <[email protected]> написал: > On Saturday, November 29, 2014 02:49:32 Ilya Chesnokov wrote: >> Я бы все же сказал, что это ORM, т.к. он по сути отображает таблицы в >> объекты. И даже, как я понял, автоматически определяет связи и создает >> аксессоры для объектов из связанных таблиц. > > Если в этом смысле, то это не классический 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/Cookbook.pod#Statically-Define-Your-Schema >> Можете конкретно объяснить, в чем его сходства и отличия от похожих >> модулей? В чем его преимущества, недостатки перед ними, что >> реализовано по-другому? >> В чем преимущество, например, перед DBIx::Class - кроме разве что >> (предположительно) меньшего потребления памяти? > > "Лёгкость" была главным аргументом. Легко начать использовать, создаются > "лёгкие" объекты. Простые вещи делаются просто -- на мой взгляд уже проще > некуда. Ок. Думаю, тоже можно добавить в описание модуля. >> Ничего не понял. Класс NoUser сгенерировался автоматически? Или это >> класс исключения, который описан где-то у вас в коде? Если так, то и >> пишите человекопонятный die() или croak(). > > Это класс исключения, что я думал понятно по слову throw, ошибку понял. В принципе понятно, если задуматься, но не очевидно :) Так что не помешал бы комментарий или более привычный способ сообщения об ошибке. >> Где комменты? Что делает, например, $client->filter_timestamp? > > Описано в Struct.pod. Ок. Это не очень очевидно, поэтому не помешал бы комментарий. >> И еще было бы неплохо показать схему таблиц, на которой вы все это >> запускаете. > > Описано в Struct.pod. Лучше было бы показать схему перед примерами, чтобы было понятнее. >> Я не знаю, как комментировать уже существующий код на гитхабе (если >> это не пулл реквест), поэтому отпишусь здесь. >> >> Во-первых, имя модуля. Я понимаю, что вы когда-то использовали (и >> может быть используете) Class::Struct, и вам понравился его синтаксис, >> но по-моему сходства между двумя модулями практически нет - так что не >> вижу причины иметь Struct в названии. Впрочем, еще меньше причин иметь >> там SQL - поскольку модуль не работает непосредственно с SQL, а скорее >> с БД, поэтому ему больше подойдет имя вида DBIx::$something. > > Зависит от точки зрения. Я рассматривал модуль как предсказуемый построитель > SQL-запросов к базе, т.е. когда я пользуюсь модулем, я представляю какой > запрос был сгенерирован на самом деле, поэтому "SQL::". Использование "Struct" > отражает принцип хранения данных, хотя, конечно, слабоватое основание. Но все же модуль этот - расширение над DBI, поэтому DBIx. Если бы он работал непосредственно с SQL, без соединения с БД, то был бы SQL:: (может я ошибаюсь - есть ли на CPAN подобные модули, называющиеся начиная с SQL::?). > >> Не заметил в коде ни одного комментария (закомментированный "#print >> $eval_code" не считается). Я понимаю, что можно писать >> самодокументируемый код, но это явно не ваш случай - код написан >> сплошняком, много повторяющихся конструкций (напр.: (index($table, " >> ") == -1)), которые неплохо бы выделить в подпрограммы. >> Все это затрудняет понимание кода - как в процессе ревью, так и при >> попытке что-то изменить / исправить баг и т.д. (к слову, sub >> setup_row() - это вообще один большой пиздец). > > Я согласен про код и setup_row() особенно. Моя проблема в том, что я не могу > себе представить потенциального читателя моего кода, а лично для меня не > представляет проблемы его прочтение и через несколько лет. "Когда пишете код, представляйте, что следующий, кто будет его поддерживать - сумасшедший маньяк, который знает, где вы живете" :) > Комментарии имеет > смысл писать в уже статичном коде, в динамично меняющемся коде они могут > устаревать и вводить в заблуждение. Комментарии нужно писать постоянно и менять с изменением кода. Или писать самодокументированный код: например, вместо $client->filter_timestamp писать $client->strip_microseconds_from_timestamp_columns (я не предлагаю это делать - только привожу пример, как это могло бы выглядеть более читабельно). > Но моя проблема в том, что после > стабилизации одного кода, я перехожу к следующему коду, оставляя написанный > без комментариев :) Для представляемого на обсуждение кода нужно подумать как > поменять привычки. > >> Не вижу документации на параметры импорта. > > Её нет. Не написал пока. Это вобщем новая функциональность, документацию > добавлю. > >> К слову, о самой функции импорта: > >> >> > sub import { >> > >> > my $defconn = 0; >> > for (my $i = 1; $i < @_; ++$i) { >> > >> > if ($_[$i] eq 'connector') { >> > >> > (undef, $connector_module) = splice @_, $i, 2; >> > --$i; >> > if (not $defconn >> > >> > and check_package_scalar($connector_module, 'conn')) >> > >> > { >> > >> > *conn = \${ $connector_module . '::conn' }; >> > >> > } >> > >> > } elsif ($_[$i] eq 'constructor') { >> > >> > (undef, $connector_constructor) = splice @_, $i, 2; >> > --$i; >> >> >> - имхо, изврат - почему бы не сделать my %arg = @_; и дальше >> проверять, что там в %arg? Так ли уж нужен вам этот >> >> > $_[0]->export_to_level(1, @_); >> >> в конце? > > Он нужен, чтобы отработал Exporter. Это понятно, я скорее имел в виду, нужен ли там последний параметр - @_? Насколько я понял, вы из него уже вытащили все вам необходимое. > Можно, конечно, всё руками экспортировать, > а можно через Exporter, я выбрал последний вариант. Вернее, я не избавился от > него после добавления кастомного варианта import(). На счёт %args -- это > сломает вариант use SQL::Struct qw($conn). Не заметил варианта такого использования. Ок, если так. >> Об ошибках: >> >> croak \%error - это далеко не самый удобный для пользователя способ >> получать сообщение об ошибке. Что он увидит на экране, >> HASH(0x2558cb8)? В качестве одного из наиболее гибких способов >> уведомления об ошибках можно рассмотреть модуль >> https://metacpan.org/pod/Lexical::Failure. >> Еще об ошибках: у вас везде { result => 'SQLERR' } - может в таком >> случае вообще избавиться от поля result? > > Да, ошибки немного "оригинально" сообщаются. Возможно имеет смысл добавть > встраиваемый механизм. Я ловлю и проверяю их на ref $@, в структурированном > сообщении больше полезной информации. Сам по себе механизм неплох, но не очень привычен для CPAN-модуля. К слову о пространствах имен - вы генерируете классы DBC::* и DBQ::* - где гарантия, что в коде не будет уже классов с таким именем? Может стоит добавить имя базового модуля в качестве префикса - т.е. что-то типа SQL::Struct::DB[C|Q]::*? >> Да, и зачем croak(), если die() имеет тот же эффект с хешами? > > Привычка, да и само слово не означает "умереть" в буквальном смысле. Зато die() побыстрее. >> О коде: >> > sub connect { >> > goto &_not_yet_connected; >> > } >> >> >> А зачем такой изврат? Почему бы не сделать просто что-то типа return >> state $connection ||= _connect();? > > Личное предпочтение. Именно шаблон предлагаемого кода я считаю > "технологическим извратом", а подмену функции на возвращающую значение > безусловно считаю "умно". Это безусловно "умно", а следовательно нарушает принцип KISS. >> По моему нескромному мнению для подобного гораздо лучше подходит >> here-document. > > Ещё лучше подошёл бы набор шаблонов, но требовать шаблонизатор для подобного > модуля уже перебор :) Особой разницы не вижу, но мне не принципиально. Шаблоны - это точно перебор, а heredoc - самое то. Плюс Sub::Quote. >> >> ---- >> >> Ну в-общем как-то так. >> >> По моему мнению модуль для публикации на CPAN не готов. > > Это, вобщем, я и ожидал. Не стоит это воспринимать как приговор - это всего лишь первое ревью :) >> Инглиш вполне сносный. Лучше напишите на русском так, чтобы все было >> понятно. А на английский потом перевести нетрудно. Заодно и >> презентацию можно будет сделать потом при желании. > > На самом деле, этот модуль, в общем-то, "мелочь", на которой я хотел > опробовать свои навыки в публикации кода для широкой общественности. Отлично. Исправляйте основные проблемы и публикуйте. По большому счету единственная серьезная проблема - это 30-секундное ожидание при отсутствии соединения с базой. Также нужно решить вопрос об имени модуля и пространстве имен генерируемых классов. Остальное в порядке "желательного". Даже error reporting можно оставить как есть - только описать его в документации. Да, еще заметил: > $conn = $connector_module->$connector_constructor(@$connector_args) > or croak { > answer => "SQL_connect error", > result => 'SQLERR', > }; - везде 'message', а тут 'answer' - ошибка? > Особой > какой-то презентации и продвижения его не планировал. Мне достаточно, чтобы в > будущем можно было делать apt-get install libsql-struct-perl (впрочем, о > названии ещё подумаю) в своём дистрибутиве, это всё, что мне нужно. Для своего дистрибутива можно создать отдельный репо, как многие делают. > -- > PEF Developer > -- > Moscow.pm mailing list > [email protected] | http://moscow.pm.org -- Best regards, Ilya Chesnokov -- Moscow.pm mailing list [email protected] | http://moscow.pm.org
