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

Make Pretty Printing more Configurable #436

Merged
merged 22 commits into from
Nov 10, 2019
Merged

Make Pretty Printing more Configurable #436

merged 22 commits into from
Nov 10, 2019

Conversation

PEZ
Copy link
Collaborator

@PEZ PEZ commented Oct 28, 2019

What has Changed?

The pretty printing through nrepl does more than just pretty printing the results, it expands reader literals and values. At least for all built-in printers, except clojure.core/pprint. This seems non-trivial to fix so this PR tries to give the users options to ”pick her poison”. I plan to offer a settings something like so:

  • Pretty print using:
    • clojure.core/pprint, this leaves reader literals and values alone, but the pretty printing is limited
    • fipp, expands reader literals and chokes on e.g. Datomic :db values, but the a decent pretty printer
    • puget, expands reader literals and values, popular pretty printing engine
    • server side zprint, same effect on the data as puget, advanced and configurable pretty printing.
    • client side, using zprint, leaves the data alone, chokes on e.g. Datomic :db values when parsing to EDN (and prints plain if so). Pretty printing is great when the data parses to EDN.

With zprint there is hope for much improvement, because @kkinnear is looking at it. See this issue: kkinnear/zprint#111

Depending on wether, and how, this is fixed in zprint this PR might not be needed. Or rather change to just using zprint as the pprint engine, w/o the user getting to choose it.

Fixes #415 #378

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.) You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. (For now you'll need to opt in to the CircleCI New Experience UI to see the Artifacts tab, because bug.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse

@PEZ PEZ added pprint Pretty printing issues enhancement labels Oct 30, 2019
@PEZ PEZ changed the base branch from master to dev October 30, 2019 12:52
@PEZ PEZ changed the title [WIP] Offer some different ways to pprint Make Pretty Printing more Configurable Nov 9, 2019
@PEZ
Copy link
Collaborator Author

PEZ commented Nov 9, 2019

Right, so now I have implemented this so that #415 is taken care of in what I think is a user friendly way. Please have a look at the changes and also put them to some testing, because I had to change stuff in quite a lot of places in order to get the pprint configuration in place. I might have broken something, but nothing that I find in my testing at least.

To understand the changes I think it is best to start with the added pprint documentation.

@cfehse
Copy link
Contributor

cfehse commented Nov 10, 2019

@PEZ

Why do we not simplify the configuration a bit like this and have only one setting for the print engine where calva is client and all other choices are the server ones. I find this a bit easier to understand:

"calva.prettyPrintingOptions": {
        "type": "object",
        "description": "Settings for Calva's pretty printing",
        "properties": {
            "enabled": {
                "type": "boolean",
                "description": "Should evaluations be pretty printed?"
            },
            "printEngine": {
                "type": "string",
                "description": "The print engine to use. ",
                "enum": [
                    "calva",
                    "pprint",
                    "fipp",
                    "puget",
                    "zprint"
                ]
            },
            "width": {
                "type": "number",
                "description": "The width of the printing. Or line length... Hmmm, you get it, I hope."
            },
            "maxLength": {
                "type": [
                    "number",
                    "null"
                ],
                "description": "The maximum number of forms to print of each collection. For no limit, do not include this in your setting. See https://clojuredocs.org/clojure.core/*print-length*"
            },
            "maxDepth": {
                "type": [
                    "number",
                    "null"
                ],
                "description": "The maximum number of levels deep the printing will go in nested objects. For no limit, do not include this in your setting. Does not exist for `puget`. See also https://clojuredocs.org/clojure.core/*print-level*"
            }
        },
        "required": [
            "enabled",
            "printEngine"
        ],
        "default": {
            "enabled": true,
            "printEngine": "calva",
            "width": 120
        }
    }

@PEZ
Copy link
Collaborator Author

PEZ commented Nov 10, 2019

Yeah, that's much better. I'll fix!

@PEZ
Copy link
Collaborator Author

PEZ commented Nov 10, 2019

There. Great that you saw how the confusion could get removed!

@PEZ PEZ merged commit 3b390b3 into dev Nov 10, 2019
@PEZ PEZ deleted the wip/configurable-pprint branch November 10, 2019 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pprint Pretty printing issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants