bondage.js icon indicating copy to clipboard operation
bondage.js copied to clipboard

Solves issue #67 and name changes to align with YS nomenclature

Open alforno opened this issue 3 years ago • 14 comments

Reworked the runner so that it aggregates options for a node correctly, which is to present them at the end of a node's run, after processing shortcut branches, commands and conditionals existing on prior line numbers. Added tests to exemplify the changes. The naming of classes and objects are now more easily mapped to those found in YarnSpinner. Mainly, shortcuts are shortcuts and options are options, whereas before shortcuts were options and options were links.

alforno avatar Jul 30 '20 15:07 alforno

I need to check and see what I did wrong. Tests are passing locally.

alforno avatar Jul 30 '20 15:07 alforno

Something wrong with the way I branched maybe. Closing this.

alforno avatar Jul 30 '20 16:07 alforno

The problem was an uncommitted bondage.min.js file.

alforno avatar Jul 30 '20 16:07 alforno

Not sure. Will try with a new pr.

alforno avatar Jul 30 '20 16:07 alforno

I know this looks crazy and I do hate the mess, but I believe this is now resolved. The problem was that I didn't pull down the version change to my branches. This should pass the checks now, and if not, then I will make new PR.

alforno avatar Jul 30 '20 16:07 alforno

Thank you for working on this. I need to give it a good test when I get some time in the week. Sorry its taking a while.

Do they work OK when inside an if statement

blurymind avatar Aug 03 '20 00:08 blurymind

Do they work OK when inside an if statement

You are very welcome, it's a great project. Yes, it now supports conditional options.

alforno avatar Aug 03 '20 02:08 alforno

I need to test this with my wrapper on gdevelop to make sure it hasnt broken the test project there

blurymind avatar Jan 13 '21 18:01 blurymind

The PR adds some great fixes. I left some comments and will give it a test, but would be more comfortable if another set of eyes that know the project better than me can have a look. @hylyh can you see if the code looks alright too?

blurymind avatar Jan 13 '21 18:01 blurymind

Since this will break bondagejs in existing usages (guessing if (result instanceof bondage.OptionsResult) will break), how about targeting this to another branch on this tracker? We can call it bondage-2 and use it to implement a version of bondage that supports the new yarn syntax.

@alforno @hylyh what do you think? Just a suggestion :)

blurymind avatar Jan 22 '21 18:01 blurymind

I'll make some time in the next few days to change back the names where it introduces the breaking changes. If I'm successful then I will create a new pull request.

alforno avatar Jan 22 '21 19:01 alforno

I am ok with making API breaking changes, since we can just push it as a major version update (bringing us to v3.0.0). This is a big enough change that I think it warrants it

hylyh avatar Jan 26 '21 00:01 hylyh

just wanna say it would (still) be great to have @alforno's branch on the main here!

miker2049 avatar Apr 27 '21 03:04 miker2049

Any recent action on this? Are we just waiting for merge conflicts to be fixed?

mnbroatch avatar Jun 24 '21 19:06 mnbroatch