JuliaGizmos / Escher.jl

Composable Web UIs in Julia
https://juliagizmos.github.io/Escher.jl
Other
335 stars 63 forks source link

Fix ambiguous foreach warning #132

Closed randyzwitch closed 8 years ago

randyzwitch commented 8 years ago

I realize #117 exists, but it doesn't appear it's going to be merged any time soon until all the changes are separated.

In the meantime, this one line, 8 character change is the minimum for 'Hello, World' functionality of examples\layout2.jl.

screen shot 2016-04-07 at 9 15 13 am

It'd be great if this could be merged and tagged quickly, so that Escher isn't in a broken state for 0.4 daily

MichaeLeroy commented 8 years ago

I agree that this issue is a good splitting off of one of the issues addressed in #117. And this fix should work. However, it might be wiser to take the issue to Reactive rather than patching Escher. I've played a bit with this problem in Reactive (in the hope of raising my suggestion as a PR rather than just an issue).

It is not at all difficult to modify Reactive so that it extends the generic function foreach which now becoming part of Julia. Unfortunately there is a plot complication in that that for foreach enters the language via Compat if VERSION < v"0.5.0-dev+977". So whether Reactive needs to define or extend foreach and how it is to be extended depends upon Julia version and whether one's code is using Compat. I can overcome this complication but my first pass solution is ugly, so I'd like to do better.

In broad strokes, my current thinking toward a solution is to first require that Reactive use Compat to ensure that the broadly generic foreach is defined and then to extend foreach to apply to signals in Reactive's way. How this extension is done depends upon whether foreach comes from Base or Compat. In my initial take on this I used an ugly try-catch trick. Much simpler would be to check VERSION and act accordingly. This is a mildly unsatisfactory approach in that it is an indirect determination of the source of foreach.

I'll get over my dithering soon and submit an issue or PR to Reactive. Meanwhile any wisdom would be welcome. Or if anyone wants to beat me to a good Reactive PR on this, go for it.

alinchis commented 8 years ago

I have prefixed foreach so now I have Reactive.foreach in ~/.julia/v0.4/Escher/src/cli/serve.jl it worked only partialy, from the examples files only form.jl and recursive-layout.jl work, as far as I can tell minesweeper.jl gives

WARNING: using Lazy.flatten in module Main conflicts with an existing identifier. WARNING: [a,b] concatenation is deprecated; use [a;b] instead

and the rest, show this in browser

LoadError: ArgumentError: Compose not found in path

randyzwitch commented 8 years ago

@alinchis That's a good point, there should be a pull request (preferably separate) to bring all of the examples up-to-date.

shashi commented 8 years ago

Reactive now imports Base.foreach on 0.5, tagged a new release of Reactive.

I think this PR is still a readability improvement.

MichaeLeroy commented 8 years ago

@shashi, thanks for fixing Reactive! I had arrived at a very similar solution but was concerned that specifying the inputs... of foreach as exclusively of type Signal might be too restrictive, shunting calls that contain inputs... of mixed Signal and other types to the Base version of foreach. (Work on this issue has convinced me that I need to learn a bit more of the the nuts and bolts of Julia's dispatch mechanisms.)