Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global print settings #25

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

volrath
Copy link
Member

@volrath volrath commented Dec 23, 2017

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.
  • Add print settings to elisions.

If the writter function cannot find print settings for the given message, it
uses global print settings.
This helps us use print settings of parent sessions when needed.
@cgrand
Copy link
Member

cgrand commented Dec 23, 2017

A default value may avoid a breaking change.

@volrath
Copy link
Member Author

volrath commented Dec 23, 2017

@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.

This function will elide content and will provide print-settings information for
the execution context
@volrath volrath changed the title WIP: Global print settings Global print settings Dec 28, 2017
@volrath
Copy link
Member Author

volrath commented Dec 28, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants