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

Spec for unordered tech dependencies #25

Merged
merged 1 commit into from
Jul 2, 2015
Merged

Spec for unordered tech dependencies #25

merged 1 commit into from
Jul 2, 2015

Conversation

SwinX
Copy link

@SwinX SwinX commented Jun 26, 2015

Spec has 4 parts:

  • resolve entity - entity deps for specific tech
  • resolve entity - tech deps
  • resolve tech - entity deps
  • resolve tech - tech deps

Resolves #12

@SwinX
Copy link
Author

SwinX commented Jun 26, 2015

@blond посмотри пожалуйста. Предлагаю ревью делать по каждому файлу отдельно. Т.е. пока 1 полностью не вычитаем - к следующему не переходить.

@SwinX
Copy link
Author

SwinX commented Jul 1, 2015

Убрал лишние describe.
cc @blond

expect(resolved.entities).to.contain({ block: 'B' });
});

it('should include entity once if entity depends on tech of itself', function () {
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
Author

Choose a reason for hiding this comment

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

Думаю, можно убрать, он дублирует уже проверенное.

@blond
Copy link
Member

blond commented Jul 1, 2015

unordered_dependencies/entity-entity.spec.js

Слова в названии файлов пишем через минус ;)

Вроде бы можно оставить только слово unordered, смысл поменяться не должен.

);
});

it('should resolve multiple tech in entity depending on tech in another entity and this tech matches ' +
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
Author

Choose a reason for hiding this comment

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

Что несколько технологий в 1 сущности могут зависеть от другой технологии и если эта технология совпадает с раскрываемой сейчас - она будет включена и попадёт в entities

Copy link
Member

Choose a reason for hiding this comment

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

ок

@blond
Copy link
Member

blond commented Jul 1, 2015

Написал замечания и вопросы.

По кейсам их tech-tech давай обсуждать после того, как остальные доделаются.

@SwinX
Copy link
Author

SwinX commented Jul 1, 2015

Вроде бы можно оставить только слово unordered, смысл поменяться не должен.

👍

opts = { tech: 'css' },
resolved = resolve(decl, deps, opts);

expect(resolved.dependOn).to.be.contain({
Copy link
Member

Choose a reason for hiding this comment

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

Мы никак не проверили, что B не попадёт в полу entities.

Copy link
Member

Choose a reason for hiding this comment

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

Из формулировок should resolve не очень понятно на какой именно результат мы хотим проверить. Может быть стоит все тесты в tech-tech разбить на 2 группы: проверка entities, проверка dependOn?

@SwinX
Copy link
Author

SwinX commented Jul 2, 2015

Распилил tech-tech спеку на 2 части, убрал лишние тесты.
@blond посмотри пожалуйста

…sections: entity - entity for tech, entity - tech, tech - entity and tech - tech dependencies
@SwinX
Copy link
Author

SwinX commented Jul 2, 2015

Поправил tech-tech зависимости.
/cc @blond

blond added a commit that referenced this pull request Jul 2, 2015
Spec for unordered tech dependencies
@blond blond merged commit 3faa43e into master Jul 2, 2015
@blond blond deleted the issue-12 branch July 2, 2015 14:10
@blond blond removed the review label Jul 2, 2015
@blond
Copy link
Member

blond commented Jul 2, 2015

👍

@blond
Copy link
Member

blond commented Jul 2, 2015

/cc @tadatuta

resolve = require('../../../lib/index').resolve;

describe('resolving unordered dependencies: entity - tech', function () {
it('should resolve entity depending tech in another entity', function () {
Copy link
Member

Choose a reason for hiding this comment

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

should resolve entity depending tech in another entity
по-русски ожидалось «должно резолвить сущность, которая зависит по технологии от другой сущности»?
если так, то я бы написал
should resolve entity depending by tech on another entity

и далее по коду я бы интуитивно писал «depending by tech on»

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.

Resolve spec -> unordered tech dependencies
3 participants