search

BetterThanTomorrow / joyride

Making VS Code Hackable like Emacs since 2022
https://marketplace.visualstudio.com/items?itemName=betterthantomorrow.joyride
Other
469 stars 17 forks source link

Use pprint/write instead of pprint/pprint #109

Closed PEZ closed 1 year ago

PEZ commented 1 year ago

The Clojure/Java nrepl server uses a wrapper around pprint/write when pretty printing. Thinking we can do as well, except I didn't see a need for wrapping.

Also adding clojure.core/pr as a pretty print function.

Fixes:

borkdude commented 1 year ago

I don't know what problem this fixes. Can you give an example?

borkdude commented 1 year ago

Sorry, I see the original issue. But why is this fixed by mapping one function to a function that's not the same? Isn't this indicative of the problem being elsewhere perhaps?

borkdude commented 1 year ago

Perhaps this is the problem: https://github.com/BetterThanTomorrow/joyride/blob/9938140f3ba512f3d477135b7234180aa0669993/src/joyride/nrepl.cljs#L92

PEZ commented 1 year ago

Perhaps this is the problem:

https://github.com/BetterThanTomorrow/joyride/blob/9938140f3ba512f3d477135b7234180aa0669993/src/joyride/nrepl.cljs#L92

It doesn't seem so. I tried it now and it doesn't seem to have an effect either way, and regardless of pretty printing of not. Could mean this is partly an upstream issue, but there is no way around that cljs.pprint/pprint adds a newline, I think:

https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/pprint.cljs#L825

So, that's why I mapped it directly to write.

borkdude commented 1 year ago

I tried it now

What exactly did you try?

Before merging this PR, I'd like to have a look myself because the current solution feels like working around something without properly finding the cause.

borkdude commented 1 year ago

I can take a look later today

PEZ commented 1 year ago

What exactly did you try?

I tried with binding sci/print-newline false still using pprint. Not expecting that to make a difference since I can see that pprint adds a newline, and didn't. Full disclosure: I Haven't had a look at what sci does with this binding, but my imagination can't come up with a thing it could do to mitigate what seems to me as the root cause.

Please have a look. I certainly agree about solving this the right way.

borkdude commented 1 year ago

I did the following for nbb:

https://github.com/babashka/nbb/commit/5fcc09a632cd7038539032a6fd3e8855f902fe63

I think we should do the same for joyride (as this is basically a copy of that code ;))

PEZ commented 1 year ago

✅