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.

Sign up for GitHub

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

Jump to bottom

The Buses #1392

Merged
merged 7 commits into from
Aug 1, 2018
Merged

The Buses #1392

merged 7 commits into from
Aug 1, 2018

Conversation

evgenykochetkov
Copy link
Contributor

@evgenykochetkov evgenykochetkov commented Jul 30, 2018 •
edited
Loading

Closes #1353,
Closes #1364

@evgenykochetkov evgenykochetkov self-assigned this Jul 30, 2018
@evgenykochetkov evgenykochetkov requested a review from nkrkv July 30, 2018 09:08
@evgenykochetkov evgenykochetkov force-pushed the feat-1353-buses branch 2 times, most recently from da3e697 to bf96ea0 Compare July 30, 2018 09:52
@evgenykochetkov evgenykochetkov force-pushed the feat-1353-buses branch 2 times, most recently from 5ad3efd to b768bd3 Compare July 30, 2018 11:12
nkrkv
nkrkv requested changes Jul 30, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

🔥 No chances for such a big PR to be approved right away, right? :)

packages/holes/src/Holes_Map_String.rei Outdated

let fromDict: Js.Dict.t('v) => t('v);

let toDict: t('v) => Js.Dict.t('v);
Copy link
Member

Choose a reason for hiding this comment

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

Every symbol in a *.rei should have a doc-string, even if it is quite simple.

packages/xod-project/src/Buses.rei Outdated

let jumperizePatch: (Patch.t, matchingBusNodes) => Patch.t;

let jumperizePatchRecursively: (Project.t, Patch.path) => Project.t;
Copy link
Member

Choose a reason for hiding this comment

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

All *.rei symbols have to be documented 😭

packages/xod-project/src/Link.re Outdated
[@bs.module ".."]
external create :
(~toPin: Pin.key, ~toNode: Node.id, ~fromPin: Pin.key, ~fromNode: Node.id) =>
t =
"createLink";

[@bs.module ".."] external getId : t => id = "getLinkId";
Copy link
Member

Choose a reason for hiding this comment

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

Chris Carter would call it [@bs.outthere] 👽

packages/xod-project/src/TypeDeduction.re Outdated
|. Belt.Map.String.remove(pinKey);

if (Belt.Map.String.isEmpty(updatedNodeTypes) ) {
drs |. Belt.Map.String.remove(nodeId);
Copy link
Member

Choose a reason for hiding this comment

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

Bet it will be more clear to just open the Belt… For the whole module

packages/xod-project/src/TypeDeduction.re
};
},
);
};
Copy link
Member

Choose a reason for hiding this comment

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

EOL is missing.

😮 and a corresponding *.rei is missing too

@nkrkv nkrkv changed the title Buses The Buses Jul 30, 2018
@evgenykochetkov evgenykochetkov force-pushed the feat-1353-buses branch 3 times, most recently from 480d6f7 to 98371af Compare July 30, 2018 15:17
nkrkv
nkrkv approved these changes Jul 30, 2018
View reviewed changes
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

👏

Vote for moving the jumper node to xod/patch-nodes and LGTM!

brusherru
brusherru approved these changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@brusherru brusherru left a comment

Choose a reason for hiding this comment

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

Just two remarks, in the rest LGTM.

packages/xod-project/src/Buses.re Outdated

let toBusNodesByLabel =
nodes
|. List.keep(n => Node.getType(n) == "xod/patch-nodes/to-bus")
Copy link
Contributor

Choose a reason for hiding this comment

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

Better use constant or function from JS-part of xod-project

packages/xod-project/src/Buses.re Outdated
[]; /* short-curcuit for optimization */
} else {
nodes
|. List.keep(n => Node.getType(n) == "xod/patch-nodes/from-bus")
Copy link
Contributor

Choose a reason for hiding this comment

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

The same one.

packages/xod-project/src/project.js
@@ -1074,6 +1075,7 @@ const getDependenciesMap = (project, patchPaths, depsMap) => {
const currentPatchDeps = R.compose(
R.map(Node.getNodeType),
Patch.listNodes,
// TODO: make safer?
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good :)

nkrkv
nkrkv reviewed Aug 1, 2018
View reviewed changes
packages/xod-project/src/patch.js Outdated
@@ -1156,7 +1158,7 @@ export const isGenuinePatch = def(
'isGenuinePatch :: Patch -> Boolean',
R.compose(
R.not,
R.anyPass([isAmong(BUILT_IN_PATCH_PATHS), isTerminalPatchPath]),
R.anyPass([R.has(R.__, BUILT_IN_PATCHES_EXCEPT_TRMINALS), isTerminalPatchPath]),
Copy link
Member

Choose a reason for hiding this comment

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

Typo in trminls

@evgenykochetkov evgenykochetkov merged commit 01bfd33 into master Aug 1, 2018
evgenykochetkov added a commit that referenced this pull request Aug 10, 2018
@evgenykochetkov
fix(xod-client): fix displaying pins with conflicting types as custom
256adff 
Bug was introduced in #1392
@evgenykochetkov evgenykochetkov mentioned this pull request Aug 10, 2018
Merged
@nkrkv nkrkv mentioned this pull request Aug 17, 2018
Merged
@evgenykochetkov evgenykochetkov deleted the feat-1353-buses branch August 18, 2018 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkrkv nkrkv nkrkv approved these changes

@brusherru brusherru brusherru approved these changes

Assignees

@evgenykochetkov evgenykochetkov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evgenykochetkov @nkrkv @brusherru