Skip to content

Conversation

@nixel2007
Copy link
Member

Fix #51

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

Copy link
Member

Choose a reason for hiding this comment

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

Я предлагаю сразу выбрасывать исключение, потому что

  • либо ошибка реализации
  • либо сборка вообще не имеет смысла из-за неверных данных

Copy link
Member Author

Choose a reason for hiding this comment

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

А в случае рекурсивной сборки? Ошибка в одном файле должна ложить всю сборку?

Copy link
Member

Choose a reason for hiding this comment

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

Сейчас в случае ошибки будет собрано что-то без расширения, это разве нормально?

Copy link
Member Author

Choose a reason for hiding this comment

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

Это не совсем нормально. Однако же самому механизму v8unpack не важен тип файла. Он нужен только для расширения. То есть если нашему неудавшемуся файлу руками добавить верное расширение, то он запустится (в случае изначально корректной структуры).
С другой стороны это настолько маловероятный сценарий, что, думаю, действительно стоит добавить исключение, как нечто из ряда вон выходящее.

Copy link
Member Author

Choose a reason for hiding this comment

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

Поправил.

@artbear
Copy link
Member

artbear commented Nov 12, 2015

@nixel2007 Вопрос остался - какой каталог нужно указывать в качестве каталога исходников при сборке?
'src\tests\Fixture' или 'src\tests\Fixture\und'
Если последний, это нужно отобразить в справке.
Если первый, тогда код компиляции неверен, т.к., например, файл 'root' ищется сразу в каталоге исходников.

@nixel2007
Copy link
Member Author

Надо указывать Fixture.
Артур, обрати внимание, что в процедуру пробрасывается не изначальный каталог с исходниками, а уже восстановленный по переименованиям ВременныйКаталог

@artbear
Copy link
Member

artbear commented Nov 12, 2015

По каталогу исходников понятно.

Copy link

Choose a reason for hiding this comment

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

Я бы здесь еще добавил строку

ИдентификаторТипаОбъекта = НРег(СокрЛП(ИдентификаторТипаОбъекта));

так как 1С допускает наличие пробельных символов между запятыми и значениями в своем формате сериализации. И идентификаторы тоже могут быть в верхнем регистре (в ранних версиях 8.0 так и было).

@nixel2007
Copy link
Member Author

@awa15 Спасибо, добавил

Copy link
Member

Choose a reason for hiding this comment

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

ИМХО Для скорости лучше не читать весь файл, а считать только 3 строки построчно.

Copy link
Member Author

Choose a reason for hiding this comment

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

Есть уверенность, что четвертый элемент всегда будет в третьей строке?

Copy link
Member

Choose a reason for hiding this comment

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

Посмотрел разные файлы, есть эта закономерность.
Считай 5-7 строк для гарантии :) все лучше, чем весь файл считывать.
Я посмотрел precommit1c\src\tests\Fixture\und\a981feb3-1026-4b79-b758-d406f4e487a1, размер 4К, 169 строк
прочие файлы смотрел, там объем много меньше.

Copy link
Member Author

Choose a reason for hiding this comment

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

Готово

artbear added a commit that referenced this pull request Nov 12, 2015
Fix #51. Добавлено автоматическое определение типа файла при сборке из исходников
@artbear artbear merged commit c2e4dd8 into xDrivenDevelopment:develop Nov 12, 2015
@nixel2007 nixel2007 deleted the issue-51 branch November 12, 2015 22:03
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.

3 participants