Show code eval in repl option

[Cyrik]

What has Changed?

• Added the showEvalCode option
• Added ->code to status bar
• evaluateCode function can take showEvalCode as an extra option to overwrite the setting (useful for custom commands that don't really make any sense in the repl window, going to use that for custom tooltips later)

Fixes #1465

My Calva PR Checklist

I have:

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• Added to or updated docs in this branch, if appropriate
• Tested the VSIX built from the PR (so, after you've submitted the PR). 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.
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • If I am fixing the issue, I have used GitHub's fixes/closes syntax
  • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• Created the issue I am fixing/addressing, if it was not present.

Ping @PEZ, @bpringe

[Cyrik]

The ->code string could probably be something better, but I don't really know what. Otherwise, it was pretty straightforward to implement.
I'd like to clean up the evaluate.ts a little, but I'd probably break some features I don't even know about. I might try my hand at writing some tests for it, although all the side effects are difficult to catch/mock.

[PEZ]

Thanks! Now trying it. Some spontaneous comments:

• The code is replicated also when I evaluate code in the REPL window. I think it shouldn't.
• I think the code should be added to the REPL history.
• If we add the code to the REPL history, the command gets similar to the ”send to repl” commands:
  • Do we really need those commands then?
    • They cause confusion today for some users who come from Cursive where the evaluate command is named like that.
    • With the option you are adding, possibly more confusion will ensue.
    • Those commands do not evaluate the code. Selecting the current form, copy/paste it to the REPL window is not much work.
  • If we remove those commands, we could name this option ”Also send evaluated code to the REPL Window” or something like that. The current name is a bit vague about what show means, I think.
• Some other keyboard shortcuts for the REPL window use ctrl+o combos. Maybe it makes sense that this one does too? I think the the one you choose also works.
• The status bar button... Tricky. Maybe this option isn't all that much like pprint in that it needs to be always visible? What use cases for this option do we see, apart from it being a general preference?

Something to keep in mind: Sometimes people ask for an option to not display evaluations inline. (So far, no-one has found it important enough to send a PR, and maybe I haven't really encouraged it because it's one more setting.) If we would add such an option I imagine it would be something like:

• calva.evaluationDisplayResults
  • inline
  • outputWIndow
  • inlineAndOutputWindow

And then that would play well with this option if it was named something like calva.evaluationSendCodeToOutputWindow. That's a mouthful, but also quite clear.

[bpringe]

Adding my 2 cents here.

If we remove those commands, we could name this option ”Also send evaluated code to the REPL Window” or something like that. The current name is a bit vague about what show means, I think.

I agree that adding something like "repl window" is helpful for clarity.

@PEZ Is there a reason for the calva.evaluation* naming convention? If just looking at calva.evaluationDisplayResults itself, I think calva.displayEvaluationResults might be clearer. But then that, without the values for context, looks more like a boolean 😄 . Without context, something like calva.evalResultsDisplayLocation might make sense. 🤷‍♂️

[PEZ]

Good points about the settings name suggestions, @bpringe ! The reason to keep these particular two together with the same prefix is that it will show them together in the settings UI. The reason to name it evaluation and not eval is more stylistic. I know we have a mix, but I think abbreviations can often be avoided. Totally agree my suggestion for that hypothetical setting made it sound like a boolean. It's the word ”display” that seems to make it so. How about calva.evaluationResultsLocation?

[Cyrik]

I like calva.evalResultsDisplayLocation or calva.evaluationResultsOptions and add a fourth option for no-display.
I'm not sure we need it, but it would be consistent to have no-display options for both directions.
If we name it evaluationResultsOptions we could make it into an object if there are any more sub-options at some point.

I'll try to get the other changes in today. The hotkey was just the first free one I found. I'm very open to something better like ctrl+o, something.
As for the status bar icon: I think we should scrap it for now and just leave the hotkey. At some point I'd like a "evalWindowOptions" status button that opens a small modal, similar to the "feedback for vscode" button in the bar. That way we can put in all these options without overwhelming the bar.

[PEZ]

I think no-display risks being a trap and create problems.

[Cyrik]

I think no-display risks being a trap and create problems.

I can see that happening. Then I'll only add the no-display stuff as an option on snippets, not as a general option.

[Cyrik]

• fixed code replication from repl window. I've created an extra command for this so we can treat it differently than the default eval form command
• didn't even know about the history, added it now
• renamed the option and changed the description
• changed the shortcut to ctrl+alt+o s
• removed the status bar display(feels a bit strange to not know if it's active, but you see it on your first eval anyway)

I haven't removed the other options yet to keep the commit/PR easier to diff. If you still want to remove them I'll do it in a new commit or a second PR.
Ah, I'd also implement calva.evaluationResultsLocation separately otherwise this PR will be held back needlessly, unless you want it in here.

[PEZ]

This is great stuff. I left a suggestion above and then there's the merge conflict. Then we're good to go!

As for the send-to-repl commands. I think it might still be useful to have them, but we can rename them to something like Paste current form in the REPL window. But I agree with you, let's do it separately so that we don't get stuck on the wording and stop this greatness from merge.

[Cyrik]

Rebased + changed the hotkey

[PEZ]

LGTM!