-
Notifications
You must be signed in to change notification settings - Fork 31
Рефакторинг под app-template #52 #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/Модули/ПараметрыСистемыOpm.os
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хелп?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ну это можно. Мне так не принципиально.
|
Ничего такой "начальный рефакторинг" :)) |
|
Начальный, потому как не пересматривал классы, оставил как есть. Только к виду app-template привел |
EvilBeaver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Есть небольшие замечания, а так все здорово, спасибо!
P.S. надеюсь тесты проходят?
src/opm.os
Outdated
| // | ||
| //The MIT License (MIT) | ||
| // | ||
| // Copyright (c) 2016 Andrei Ovsiankin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не согласен. Во-первых, Opm идет под лицензией Апач. Откуда тут MIT? Во-вторых, моих именных копирайтов тут не надо, как мне кажется. Oscript-Library это же "Организация". Может ее и указывать?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хм.. Было из шаблона. Взято. Получается у всех разная лицензия.
src/Классы/КомандаOpm_Config.os
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мы так и не перешли на настройки в стиле git? Мы плохие ((
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У рефакторинга была другая цель. Только рефакторинг.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У рефакторинга была другая цель. Только рефакторинг.
Очень правильная цель.
Схема должна быть следующая - сначала рефакторим, по дороге фиксируем все выявленные нестыковки, непонятки, странности в виде ишузов, создаем PR, ревьюим его, дорабатываем по замечаниям, принимаем PR, и вот только тогда исправляем и дорабатываем по ишузам
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вот здесь большая просьба использовать стандартные возможности cmdline по выводу справки. Там уже есть эти расчеты ширины и прочее.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EvilBeaver Поясни, т.к. модуль полностьювзять из шаблона app
|
У меня руки так и не дошли проверить PR. |
|
Вроде бы конфликта с #61 нет, должно смержиться без проблем. |
|
|
@dmpas а у тебя были изменения в УстановкаПакета :) |
|
@nixel2007 @EvilBeaver @artbear конфликты устранили |
|
@nixel2007 @EvilBeaver @artbear ну хоть слово??? |
|
Смогу посмотреть либо в пятницу вечером, либо в только в выходные |
Начальный рефакторинг по задаче #52