unrepl icon indicating copy to clipboard operation
unrepl copied to clipboard

Global print settings

Open volrath opened this issue 7 years ago • 3 comments
trafficstars

Fixes #17.

The best way to review this PR is to check out the first two commits, which contains all of the new logic to the feature. The third commit has a huge diff but very little new stuff to it, because it's basically adding an optional parameter to unrepl/start, which reindent the whole function. The only three changes in that whole commit can be found here, here, and here.

Implementation detail:

  • Add a print-settings map to session state that has the following keys: :eval :out :err :log :exception, and for each these keys, there's a map with :string-length :coll-length :nesting-depth.
  • write-here will now try to find the print context out of the given message x. If it exist and it's part of the print-settings it will bind the print variables accordingly, if not, it will use the default (see default-print-settings
  • Add parent-session-id to unrepl/start so that aux sessions can fetch print settings from parent sessions.

Stuff missing:

  • [ ] :print-settings session action changed its signature to: [context string-length coll-length nesting-depth], which is a breaking change and should be added to the README.
  • [x] Add print settings to elisions.

volrath avatar Dec 23 '17 11:12 volrath

A default value may avoid a breaking change.

cgrand avatar Dec 23 '17 13:12 cgrand

@cgrand yeah I thought of that, but we would have to define a way to declare (as data) optional parameters in session action templates, and (hopefully) it should not be something too complicated so that clients that are not written in Clojure(Script) are able to parse it and understand it, like specs.

Another option I thought of was creating a new name for it, and leave print-limits as it was, but maybe too much of a hassle?

In reality, the API I'd like to expose for clients would be something similar to

(update-print-limits context setting-name value [setting-name-2 value-2 ...])

But I couldn't decide how to expand our representation of parameters.

I also avoided (print-limits context settings-map) cause, so far, I do not have an easy way to transform an elisp alist into a clojure map, except by making the AST node by hand or by creating the map as a string and parsing it.

volrath avatar Dec 23 '17 14:12 volrath

I'm unWIP-ing this cause I think it's ready. I'm not sure what would you want to do with the breaking change, but if we decide to add it, I'll just add the correct date to the README when you give green light to this PR.

Btw, I know that it might be hard to review start changes here because of its reindentation (basically because of the new parameter I added), if you prefer, I could revert that change and add it later, so that the diff only show the things I actually changed in start, which are not that many.

volrath avatar Dec 28 '17 23:12 volrath