Skip to content
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

real tech inheritance #230

Closed
wants to merge 2 commits into from
Closed

real tech inheritance #230

wants to merge 2 commits into from

Conversation

escaton
Copy link
Contributor

@escaton escaton commented Jun 6, 2015

Из-за _copyAnd красивое решение не придумалось, так что насчет вот такого:

module.exports = require('someTech').buildFlow()
    .builder(function() {
        return this.__base.apply(this, arguments);
    })
    .methods({
        append: function() {
            return this.__base.apply(this, arguments)
        }
    })
    .createTech(require('someTech'));

?

UPD
таки придумалось, в конце не надо снова прокидывать ту же технологию.

@escaton
Copy link
Contributor Author

escaton commented Jun 6, 2015

cc @blond
Код выглядел так, будето наследование намеренно хотели запретить. Может что-то ломается? Проверял на сборке совего проекта, вроде все ок.

@blond
Copy link
Member

blond commented Jun 6, 2015

Привет!

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

Мне кажется, что это могло быть сделано для builder из-за работы с кешом.

Причин для того, чтобы запрещать __base для обычных методов я не вижу.

@mdevils, не вспомнишь, было ли это сделано специально, и если да, то для чего?

@blond blond added the review label Jun 6, 2015
@escaton
Copy link
Contributor Author

escaton commented Jun 7, 2015

Все-таки прокидываю inheritTech через все copy так что в конец не надо добавлять .createTech(require('someTech'));

@escaton
Copy link
Contributor Author

escaton commented Jun 7, 2015

Если что, отдельно builder можно не наследовать. Вообще, кажется, build-flow можно хорошо порефачить и сделать проще. Осталось только понять, зачем на каждый чейнинг нужно создавать копию?

@mdevils
Copy link
Contributor

mdevils commented Jun 7, 2015

Копирование было сделано для того, чтобы можно было создавать новые технологии конфигурированием существующих. Вот пример: https://github.com/enb-make/enb/blob/master/techs/css-ie6.js

@escaton
Copy link
Contributor Author

escaton commented Jun 8, 2015

@mdevils если в коде этого pr убрать копирование build-flow, вроде не должно ничего сломаться?

@mdevils
Copy link
Contributor

mdevils commented Jun 8, 2015

Все должно сломаться.

Копирование — это и есть механизм наследования. Он просто был не доделан (нельзя было вызвать __base).

@escaton
Copy link
Contributor Author

escaton commented Jun 8, 2015

а зачем копировать до вызова createTech ? Ведь в нем все равно есть вызов inherit? Соответственно если в buildFlow поменять base-tech на текущую, наследование произойдет от нее?

@mdevils
Copy link
Contributor

mdevils commented Jun 8, 2015

Хм, до вызова, наверное, незачем. Можете попробовать выпилить.

@blond
Copy link
Member

blond commented Jun 9, 2015

Я попробую написать тесты (#231) на базовую функциональность build-flow, а так же на механизмы наследования и кэширования, т.к. они задеваются в данном PR.

Считаю, что вливать PR без таких тестов довольно опасно.

@blond
Copy link
Member

blond commented Jun 18, 2015

@escaton, начал писать тесты — #234.

За статусом можно следить по мета задаче — #231.

@blond blond added ready and removed review labels Jun 20, 2015
@blond
Copy link
Member

blond commented Jun 30, 2015

Я закончил писать тесты на build-flow: https://github.com/enb-make/enb/blob/master/test/lib/build-flow.js.

@escaton, можешь разбить PR на подзадачи или сформулировать задачу, которую тебе необходимо сделать, и прислать PR с тестами?

Написать тесты по аналогии с существующими должно быть не сложно. Если будут возникать проблемы — я буду помогать.

@blond
Copy link
Member

blond commented Jul 6, 2015

@escaton, проблема ещё актуальна?

@blond
Copy link
Member

blond commented Jul 13, 2015

@escaton, ping!

1 similar comment
@blond
Copy link
Member

blond commented Jul 19, 2015

@escaton, ping!

@escaton
Copy link
Contributor Author

escaton commented Jul 21, 2015

@blond сорри, вот из отпуска вернулся.
Разбивать этот PR на подзадачи мне кажется не нужно, тут только один реквест — реализовать вызов __base

@levonet levonet added review and removed ready labels Aug 3, 2015
@blond
Copy link
Member

blond commented Aug 5, 2015

@escaton, извини был немного занят

Можешь отребейзить PR и добавить к нему тестов?

@levonet levonet added in progress and removed review labels Aug 6, 2015
@blond blond added ready and removed in progress labels Aug 11, 2015
@levonet levonet added review and removed ready labels Oct 23, 2015
@qfox qfox mentioned this pull request Jan 14, 2016
@blond
Copy link
Member

blond commented Jan 14, 2016

Closed in favor of #422.

@blond blond closed this Jan 14, 2016
@blond blond removed the review label Jan 14, 2016
@blond blond added this to the 1.1.2 milestone Jan 15, 2016
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.

4 participants