Custom Printers #1141

gregoryyoung · 2016-03-22T22:20:56Z

This is not intended to be accepted but to act as a point of discussion

I have implemented here custom pretty printing into scriptcs. I felt something like this after writing it.

Its really ugly. The code does however now work. Here is an example:

greg@clown:~/src/scriptcs$ artifacts/Release/bin/scriptcs -Repl
scriptcs (ctrl-c to exit or :help for help)

> 5 + 5
10
> Repl.AddCustomPrinter<int>(x => "hello greg this is your integer " + x)
adding custom printer
> 5 + 6                                                                   
hello greg this is your integer 11
> Repl.AddCustomPrinter<int>(x => "redefining printer integer is " + x)   
adding custom printer
> 5 + 6                                                                 
redefining printer integer is 11

This is a very common thing used in the F# repl. As an example here is a little library that gives you definitions for record types http://fssnip.net/cV so it looks like

// +--------+--------+--------+
// | Label1 | Label2 | Label3 |
// +--------+--------+--------+
// | Value1 | Value2 | Value3 |
// +--------+--------+--------+
// | Value1'| Value2'| Value3'|
// +--------+--------+--------+

We make heavy use of this in privateeye

> mostCalledMethods() |> Seq.take 10;;
val it : seq<profilersession.MethodInformation> =

--------------------------------------------------------------------------------
|Name                             |  Called| Total Time|Allocs|  A. Size|Thrown|
--------------------------------------------------------------------------------
|ProcessBlock (byte[],int)        | 3176533| 16402.0126|     0|        0|     0|
|get_Chars (int)                  |  170507|   483.9711|     0|        0|     0|
|get_Rank ()                      |  140403|  2138.1667|     0|        0|     0|
|GetRank (System.Array)           |  140403|   915.1704|     0|        0|     0|
|get_Length ()                    |  131869|  2944.0053|     0|        0|     0|
|__icall_wrapper_mono_object_new_s|   81548|   922.2145| 81548|  3493132|     0|
|CachePropertyInfo (System.Reflect|   78383|   302.0055|     0|        0|     0|
|ReadSegment (byte[],int,int)     |   76117|   416.0591|     0|        0|     0|
|GetAttributeFlagsImpl ()         |   73713|   874.5466|     0|        0|     0|
|GetAttributes (System.RuntimeType|   73713|   291.3721|     0|        0|     0|
--------------------------------------------------------------------------------

Now in terms of implementation the one here is a mess but I think it can be improved upon. I don't know all the code that well, In dealing with the dependency graph:

I really don't think Repl should be exposed all the way on ScriptHost but it may actually be useful there as you can easily add quite a few other Repl configurations there. An alternative might be to isolate the dependency into ICustomPrinter and put this on ScriptHost.

Some of the rest will likely require some refactoring of dependencies and how they relate. Before just embarking on that I figured it would be worth bringing it up as a discussion.

So first question, does pretty printing seem like a reasonable feature worth following up on?

Second question, how do people see refactoring things to make dependency handling a bit more sane than it is here?

Greg

glennblock · 2016-03-23T05:34:30Z

Thanks a ton for the effort @gregoryyoung, this looks VERY COOL!. I'll take a look at the code....

glennblock · 2016-03-23T05:38:51Z

OK, I took a look. I am scared now :-) Mainly because I see you having to make changes to a bunch of our core interfaces which is a breaking change. I'll take a look and see if there is a better way / if dependency injection can help here.

glennblock · 2016-03-23T05:44:02Z

OK so first thing, is you shouldn't expose the Repl object itself to the Repl. We can add the AddCustomPrinter method directly to the Script host and then it will show up as an ambient method one can call.

glennblock · 2016-03-23T05:46:50Z

I am going to pull this down and see if I can refactor it. Then I'll send a PR to your branch.

glennblock · 2016-03-23T07:07:11Z

@gregoryyoung I am working on an approach that will move the AddCustomPrinter method to be exposed off our "env" object. It should require only changing the ScriptEnvironment / IScriptEnvironment interface if it works.

glennblock · 2016-03-23T08:03:44Z

OK, my refactoring works and it is pretty clean. PR coming.

glennblock · 2016-03-23T08:12:41Z

@gregoryyoung sent you a PR: https://github.com/gregoryyoung/scriptcs/pull/1. I started from the latest "dev" rather than from your branch, then I pulled from your code and refactored. Take a look.

If you want to test, try pulling the latest scriptcs/dev first.

gregoryyoung · 2016-03-23T15:22:32Z

Seems reasonable. As I mentioned Repl on script host didn't see right (it also caused circular dependencies which is why I had to jam in the setter).

I will try writing the equivalent of privateeye.fsx against it and see how it works

filipw · 2016-03-23T17:34:53Z

I would suggest a different route. Since we are talking about customized pretty printing, I would add this to the IConsole abstraction and expose that on the ScriptHost.

The benefit is:

"normal" script (non REPL) can choose to use it over just System.Console and also benefit from pretty print
REPL user can also use/interact with it directly if he wants too
REPL itself uses IConsole so it will just pick it up too

filipw · 2016-03-23T17:35:32Z

in fact this was brought up before, exposing IConsole, even before this particular request here. so might be the time to do it

glennblock · 2016-03-23T18:00:29Z

Exposing console is one thing and adding pretty print to the console is
another. I am not a great fan of this approach as console should be really
minimalist / a very light abstraction over the system console. It's main
reason for existence is test ability and for diff host environments.

On Wed, Mar 23, 2016 at 11:05 PM Filip W notifications@github.com wrote:

in fact this was brought up before, exposing IConsole, even before this
particular request here. so might be the time to do it

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#1141 (comment)

filipw · 2016-03-23T18:17:18Z

I am not sure if I was clear about the motivation but what I am trying to achieve is exposing something that is always usable - and can be consumed for both script and REPL.

The current solution here (adding it on the host or ScriptEnvironment) works only for REPL and has 0 effect on the script files, and yet the functionality is exposed to the script which is not ideal.

The alternative would be to simply expose pretty print on the host but only on the REPL host (so have ScriptHost and a new ReplScriptHost).

gregoryyoung · 2016-03-23T18:19:34Z

It is useful for both script and repl (though usually more useful for repl
than script)

On Wed, Mar 23, 2016 at 6:17 PM, Filip W notifications@github.com wrote:

I am not sure if I was clear about the motivation but what I am trying to
achieve is exposing something that is always usable - and can be consumed
for both script and REPL.

The current solution here (adding it on the host or ScriptEnvironment)
works only for REPL and has 0 effect on the script files, and yet the
functionality is exposed to the script which is not ideal.

The alternative would be to simply expose pretty print on the host but
only on the REPL host (so have ScriptHost and a new ReplScriptHost).

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1141 (comment)

Studying for the Turing test

glennblock · 2016-03-23T18:21:02Z

I understood what you are trying to do, though I disagree. The pretty
printing is only used when returning a value, not when you just call
Console.WriteLine. We only dump return values to the console in the REPL.

On Wed, Mar 23, 2016 at 11:47 PM Filip W notifications@github.com wrote:

I am not sure if I was clear about the motivation but what I am trying to
achieve is exposing something that is always usable - and can be consumed
for both script and REPL.

The current solution here (adding it on the host or ScriptEnvironment)
works only for REPL and has 0 effect on the script files, and yet the
functionality is exposed to the script which is not ideal.

The alternative would be to simply expose pretty print on the host but
only on the REPL host (so have ScriptHost and a new ReplScriptHost).

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#1141 (comment)

filipw · 2016-03-23T18:40:14Z

We only dump return values to the console in the REPL.

well that is my point, then it should be hidden from the ScriptHost visible in scripts, and only available on script host that's visible in the REPL

There is one more thing. IConsole is not just an abstraction for abstraction's sake, it's been used to provide real functionality - i.e. to redirect the output somewhere else like in this example over sockets to the web browser https://github.com/filipw/ScriptCs.WebConsole so there could be value in exposing it to script, allowing the script itself to tap into it

gregoryyoung · 2016-03-23T18:45:21Z

I am a bit curious on the use case mentioned for abstracting console

I know in the windows world things aren't always very composable but in the
unix world I would just redirect stdin/stderr/stdout for this (they are a
console abstraction). Is there something in the windows world (I don't know
much about it) that would prevent this?

On Wed, Mar 23, 2016 at 6:40 PM, Filip W notifications@github.com wrote:

We only dump return values to the console in the REPL.

well that is my point, then it should be hidden from the ScriptHost
visible in scripts, and only available on script host that's visible in the
REPL

There is one more thing. IConsole is not just an abstraction for
abstraction's sake, it's been used to provide real functionality - i.e. to
redirect the output somewhere else like in this example over sockets to the
web browser https://github.com/filipw/ScriptCs.WebConsole so there could
be value in exposing it to script, allowing the script itself to tap into it

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1141 (comment)

Studying for the Turing test

filipw · 2016-03-23T18:49:27Z

here are the related issues #960, #958

filipw · 2016-03-23T18:50:41Z

@gregoryyoung what you are suggesting is also possible, the idea was to have it more structured and composable, with clearly defined contracts, that's all

glennblock · 2016-03-24T06:02:29Z

And simplicity. The IConsole abstraction is easy to write unit tests
against.

On Thu, Mar 24, 2016 at 12:20 AM Filip W notifications@github.com wrote:

@gregoryyoung https://github.com/gregoryyoung what you are suggesting
is also possible, the idea was to have it more structured and composable,
with clearly defined contracts, that's all

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#1141 (comment)

awb99 · 2016-04-22T18:33:13Z

Hi gents!

I believe this is quite en important feature.

What I normally need is to quickly print a List
with some formatting options.

This is a little library I did for this purpose:
https://github.com/awb99/ObjectPrinter

glennblock · 2016-07-29T18:21:20Z

@gregoryyoung can you resolve the conflicts so I can merge this?

glennblock · 2016-09-06T02:07:41Z

Closing this as I believe #1156 addressed this.

gregoryyoung · 2016-09-06T08:02:52Z

👍

On Tue, Sep 6, 2016 at 3:07 AM, Glenn Block notifications@github.com
wrote:

Closing this as I believe #1156
#1156 addressed this.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1141 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXRWvw_YXVPv0R1EtPesTbgqLKHpZsWks5qnMrzgaJpZM4H2le2
.

Studying for the Turing test

gregoryyoung added 2 commits March 22, 2016 20:29

make giant mess.

827c7fe

break more stuff and make it work

cd302a5

glennblock closed this Sep 6, 2016

Custom Printers #1141

Custom Printers #1141

Uh oh!

Conversation

gregoryyoung commented Mar 22, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

gregoryyoung commented Mar 23, 2016

Uh oh!

filipw commented Mar 23, 2016

Uh oh!

filipw commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

filipw commented Mar 23, 2016

Uh oh!

gregoryyoung commented Mar 23, 2016

Uh oh!

glennblock commented Mar 23, 2016

Uh oh!

filipw commented Mar 23, 2016

Uh oh!

gregoryyoung commented Mar 23, 2016

Uh oh!

filipw commented Mar 23, 2016

Uh oh!

filipw commented Mar 23, 2016

Uh oh!

glennblock commented Mar 24, 2016

Uh oh!

awb99 commented Apr 22, 2016

Uh oh!

glennblock commented Jul 29, 2016

Uh oh!

glennblock commented Sep 6, 2016

Uh oh!

gregoryyoung commented Sep 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants