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

Disjoin Operation #457

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Disjoin Operation #457

wants to merge 4 commits into from

Conversation

masq
Copy link
Contributor

@masq masq commented Dec 29, 2018

Adds in the "Disjoin" operation, which attempts to satisfy the operation requested in issue #424.

I say attempts, as I'm not really sure how other types of flow operations should affect/be affected by this operation. For example, if one of the proceeding operations was a "jump" operation. I made a test out of the example provided in the issue, and that passes, but I was a little unsure of the full utility that the operation could provide, and would love to see more examples of this in order to fully flush it out.

I also heavily modeled the code based on the "Fork" operation's implementation, and there were a few things in there that I'm not sure are necessary for this operation that were present in the "Fork" operation. So, I would love a code review to look for things that were unnecessarily copied over. Just let me know! Thanks!

Comment on lines +29 to +33
{
"name": "Split delimiter",
"type": "binaryShortString",
"value": "\\n"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this operation need to split? I think I'd prefer using Fork and then Disjoin. No need to have the same capability in both.

break;
} else {
const subRec = new Recipe();
subRec.addOperations(opList[i]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

addOperations takes a list of operations, should probably be
subRec.addOperations[opList[i]]

const recipe = subRecList[i];

// Take a deep(ish) copy of the ingredient values
const ingValues = recipe.opList.map(op => JSON.parse(JSON.stringify(op.ingValues)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use structuredClone instead

constructor() {
super();

this.name = "Disjoin";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not sure how I feel about the name Disjoin. Perhaps Concurrently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants