Rendering support for explicit slandles #3340

Cypher1 · 2019-07-17T02:01:22Z

This should enable the the stardate and favorite foods slandles demos to work.

Can be tested with sigh devServer and the following urls:

http://localhost:8786/shells/dev-shell/?m=https://$arcs/particles/Profile/SLANDLESFavoriteFood.recipes
http://localhost:8786/shells/dev-shell/?m=https://$arcs/particles/Stardate/SLANDLESStardate.recipes

src/planning/strategies/match-recipe-by-verb.ts

src/runtime/recipe/handle-connection.ts

piotrswigon · 2019-07-18T07:34:41Z

src/runtime/recipe/handle.ts

+    return slandle;
+  }
+
+  _copyInto(recipe: Recipe, _cloneMap: CloneMap, variableMap: VariableMap) {


why add _ prefix?

_copyInfo needs the prefix to match the rest of the code base, I would like to remove this at some point.

The _ for the arguments has been discussed elsewhere (tslint complains) but I will stop listening to it about this.

src/runtime/recipe/handle.ts

src/runtime/recipe/particle.ts

src/runtime/recipe/recipe-resolver.ts

piotrswigon · 2019-07-18T07:40:09Z

src/runtime/recipe/recipe-resolver.ts


 export class ResolveWalker extends RecipeWalker {
+  options: IsValidOptions;


can we make it private?

Done.

I'd prefer to pass the options around explicitly, but the RecipeWalker & Walker classes don't currently support that. Should I keep track of this or file a bug?

src/runtime/recipe/recipe-resolver.ts

piotrswigon · 2019-07-18T07:53:03Z

src/runtime/recipe/slot-connection.ts

+        options.errors.set(this.name, label);
+      }
+      if (options && options.details) {
+        options.details = label; // TODO use .errors instead.


TODO(jopra) or remove

I had left this off as I do not believe that this is the responsibility of any one engineer. I think we all will benefit from being consistent in the way that we record errors. Will add it so that someone is tracking it (me).

src/runtime/recipe/slot.ts

piotrswigon

If you knew beforehand that you need better typing, it would be worthwhile to separate changes that are no-op (e.g. adding types) from the meat of this change that actually does what you describe in the change name (i.e. hiding slot connections behind methods, so that you can start implementing slandels interop - if I understand correctly). It would make an easier review if all changes were directly contributing to making slandels work.

Cypher1 · 2019-07-19T02:26:04Z

In the past I've been told that fixing things as we notice them is desired in Arcs (as opposed to other projects where separating the concerns of commits is required).
I'll try to find a better separation in future and will ask about better workflows for this.

piotrswigon · 2019-07-21T01:29:44Z

Separation of patches is interesting. On one hand it would be a pain if re-organising a change into multiple PRs took a lot of time, so if it is painful it's ok to send a PR with unrelated changes. On the other hand if you can relatively effortlessly split a change into logical steps that are naturally stacked on top of each other, it is usually a good idea I would say.

The big if in there is (A) whether there is a logical way to split change into pieces and (B) whether it can be done and you have the tooling/workflow to allow you to do that.

FWIW I use meld for visually selecting blocks of code that I pull in into a new branch when I do splitting. You can ask around how others do that.

Cypher1 requested review from shans and piotrswigon July 17, 2019 02:01

Cypher1 force-pushed the runtimeRecipeAPISlotviews branch from f2f8301 to 60341f6 Compare July 17, 2019 02:05

Cypher1 mentioned this pull request Jul 18, 2019

Show slandles in PEC overview #3350

Merged