clojure-dev: #55: clojure.contrib.sql expects *err* to be a PrintWriter Ticket updated - Resolution?

[entaroadun]

In reference to this thread on the clojure-dev group:
http://groups.google.com/group/clojure-dev/browse_thread/thread/369734ff42cbb06a/cc9e30534c78b6b3?lnk=gst&q=sql+println#cc9e30534c78b6b3

Has there been any resolution?

I'm using clojure.contrib.sql and Emacs with clojure-swank. When an
error is thrown in clojure.contrib.sql, it calls .println on *err*,
which is assumed to be a PrintWriter. However, the documentation on
core states that *err* is merely a Writer, and indeed, clojure-swank
binds *err* as a StringWriter to pass to Emacs. StringWriter doesn't
have .println, so it breaks.

The above thread suggests defining *err* as a PrintWriter instead of
as a Writer. Has this been patched, and is it official? If so, I'll
patch clojure-swank to use PrintWriter. If not, I'll patch
clojure.contrib.sql to not use println.

Yes, this probably belongs in clojure-dev, but I wanted to make sure I
actually have something to contribute to Clojure before signing up.

Thanks,
Johnny

[Richard Newman]

> The above thread suggests defining *err* as a PrintWriter instead of
> as a Writer. Has this been patched, and is it official? If so, I'll
> patch clojure-swank to use PrintWriter. If not, I'll patch
> clojure.contrib.sql to not use println.

I patched swank-clojure:

diff --git a/src/swank/core/server.clj b/src/swank/core/server.clj
index 31c25ee..972e74a 100644
--- a/src/swank/core/server.clj
+++ b/src/swank/core/server.clj
@@ -57,7 +57,7 @@

(defn- socket-serve [connection-serve socket opts]
(with-connection (accept-authenticated-connection socket opts)
- (let [out-redir (make-output-redirection *current-connection*)]
+ (let [out-redir (java.io.PrintWriter. (make-output-redirection
*current-connection*))]
(binding [*out* out-redir
*err* out-redir]
(dosync (ref-set (*current-connection* :writer-redir) *out*))

It would be nice to see this upstream.

[Steve Purcell]

+1.

Github user 'wilig' appears to have been working on this (or a very similar issue):

http://github.com/wilig/swank-clojure/commits/

Not sure if he's sent a pull request to Phil.

-Steve

[Phil Hagelberg]

On Sat, Feb 13, 2010 at 11:03 AM, Richard Newman <holy...@gmail.com> wrote:
>> The above thread suggests defining *err* as a PrintWriter instead of
>> as a Writer.  Has this been patched, and is it official?  If so, I'll
>> patch clojure-swank to use PrintWriter.  If not, I'll patch
>> clojure.contrib.sql to not use println.
>
> I patched swank-clojure:
>
> diff --git a/src/swank/core/server.clj b/src/swank/core/server.clj
> index 31c25ee..972e74a 100644
> --- a/src/swank/core/server.clj
> +++ b/src/swank/core/server.clj
>

> It would be nice to see this upstream.

Agreed! My patch backlog for swank-clojure is far bigger than I feel
comfortable with, but I've been focusing my attention on Leiningen for
the 1.1.0 release. Once that gets out I will turn to swank-clojure.

I should mention that I could use some help co-maintaining
swank-clojure. I don't feel like I have a great grasp on the codebase
right now and have just been keeping it on autopilot after figuring
out packaging issues, but having someone who's familiar with how it
works from CL would be very helpful. Even without CL experience, just
gathering up the various patches into one branch and seeing what works
and what doesn't would be very helpful too. Otherwise I'll get to it
when I have time, but it might not be very prompt.

-Phil

[Johnny Kwan]

FWIW, Richard's patch is the simplest thing that could and does possibly work. +1.

Thanks,
Johnny

[Steve Purcell]

On 13 Feb 2010, at 23:22, Phil Hagelberg wrote:

> Even without CL experience, just
> gathering up the various patches into one branch and seeing what works
> and what doesn't would be very helpful too.

I'll bite:

http://github.com/purcell/swank-clojure

In my master branch (freshly forked from Phil's repo) I've applied Richard's patch, plus the following recent branches from the swank-clojure network on github (http://github.com/technomancy/swank-clojure/network):

bmillare/master:
* bmillare/master allow start-repl to accept optional arguments to pass to start-server

digash/master:
* add pprint-eval for slime-pprint-eval-last-expression which is usually bound to C-c C-p

fbreuer/master
* change path in start-repl for Windows compatibility

hgiddens/customize-project-lib-path:
* Use `swank-clojure-project-dep-path' in `swank-clojure-project'.
* Added defcustom form `swank-clojure-project-dep-path'.
* Documentation updated.

I've tested it locally on my OS X machine, and it works fine. In particular, Richard's patch has solved some problems when compiling files that generated reflection warnings.

I can put my swank-clojure jar on Clojars if necessary; just let me know.

If this all works for you, Phil, you can just merge my master branch to slurp in the above changes; I'll refrain from adding anything else to it.

-Steve

[Steve Purcell]

On 14 Feb 2010, at 09:34, Steve Purcell wrote:

> In my master branch (freshly forked from Phil's repo) I've applied Richard's patch, plus the following recent branches from the swank-clojure network on github (http://github.com/technomancy/swank-clojure/network):

I should add that I skipped the branch from user "wilig", since it seemed to duplicate Richard's patch less simply, and also hgiddens' branch "swank-clojure-project-uses-clojure", which broke my vanilla slime/clojure install. (I think he must have had a different Slime setup, because I don't have the problem he was trying to fix.)

-Steve

[wilig]

I'm willing to give maintenance of swank-clojure a shot. With one
huge caveat: I'm very new to both clojure and CL, so patience will
have to be practiced as I slowly get up to speed on a new code base in
a new language. Unless anyone with more experience wants to jump in,
I will begin applying patches, and working on open issues.

Thanks in advance for your understanding.

Will

[Jason Samsa]

Patching swank might be appropriate but doesn't address several
incorrect assumptions made by c.c.sql:

1) *err* and *out* promise to implement java.io.Writer in the
documentation not java.io.PrintWriter. Calling .println on *out* or
*err* is not appropriate.

2) c.c.sql shouldn't assume that we want anything printed at all?!
Just throw the SQLException.

3) c.c.sql's use of throwf unnecessarily wraps the SQLException into
java.lang.Exception. Doing so isn't useful, especially if you want
handle an SQLException earlier than an unknown exception.

[Richard Newman]

> Patching swank might be appropriate but doesn't address several
> incorrect assumptions made by c.c.sql:
>
> 1) *err* and *out* promise to implement java.io.Writer in the
> documentation not java.io.PrintWriter. Calling .println on *out* or
> *err* is not appropriate.

I started in the same mental place as you. See

http://groups.google.com/group/clojure-dev/browse_thread/thread/369734ff42cbb06a/cc9e30534c78b6b3?lnk=gst&q=sql+println#cc9e30534c78b6b3

Steven asserted that this was a Clojure doc bug, not a sql bug. Given
that Steven added *err* to Clojure, I trust his judgment about what
the code is supposed to do :)

I personally still feel that switching those ".println"s to "printlns"
would be neater, but it's really no big deal.

It's also cheap to wrap a Writer in a PrintWriter, as I did with swank-
clojure.

> 2) c.c.sql shouldn't assume that we want anything printed at all?!
> Just throw the SQLException.

Printing to *err* is acceptable IMO. This is the common "should my
library write log messages, and if so how?" problem. I'd rather have
it print to *err* than use Java Logging.

> 3) c.c.sql's use of throwf unnecessarily wraps the SQLException into
> java.lang.Exception. Doing so isn't useful, especially if you want
> handle an SQLException earlier than an unknown exception.

That sounds like a reasonable complaint. Perhaps mail Steven to
discuss, or create an Assembla issue?

[Jason Samsa]

I would like to create a patch to address the issues above and get the
c.c.sql tests running under clojure.test. I should have a patch ready
by the end of the day.

I've just filled out my contributor agreement it'll be in the mail
tomorrow.

Jason

[Jason Samsa]

> > 2) c.c.sql shouldn't assume that we want anything printed at all?!
> > Just throw the SQLException.
>
> Printing to *err* is acceptable IMO. This is the common "should my  
> library write log messages, and if so how?" problem. I'd rather have  
> it print to *err* than use Java Logging.
>

Why log anything at all? The relevant information is (or can be)
stored in the exception. I don't want to see clojure contrib embrace
the catch/log/re-throw idiom. I couldn't find any other code in
contrib that follows this approach.

Jason

[Richard Newman]

> Why log anything at all? The relevant information is (or can be)
> stored in the exception. I don't want to see clojure contrib embrace
> the catch/log/re-throw idiom. I couldn't find any other code in
> contrib that follows this approach.

Ain't my code. I was simply saying I prefer libraries to print to
*err* than drag in a dependency on a bad logging library.

I suggest you write up your thoughts and start a discussion with
scgilardi...

[wilig]

> 1) *err* and *out* promise to implement java.io.Writer in the
> documentation not java.io.PrintWriter. Calling .println on *out* or
> *err* is not appropriate.

Actually this was the impetus for my change to the swank-clojure. I
still vastly prefer Richards implementation to mine, but I thought it
would be useful to catch a call to println so a warning could be
issued along with the output.

Perhaps the spec should be changed to bind *err* and *out* to a
java.io.PrintWriter? println is certainly handy.

[Terje Norderhaug]

On Feb 13, 2010, at 3:22 PM, Phil Hagelberg wrote:
> I should mention that I could use some help co-maintaining
> swank-clojure. I don't feel like I have a great grasp on the codebase
> right now and have just been keeping it on autopilot after figuring
> out packaging issues, but having someone who's familiar with how it
> works from CL would be very helpful. Even without CL experience, just
> gathering up the various patches into one branch and seeing what works
> and what doesn't would be very helpful too. Otherwise I'll get to it
> when I have time, but it might not be very prompt.

I am gearing up for considerable involvement in improving swank-
clojure. I am very familiar with CL swank, having implemented the
MCLIDE swank client - a Macintosh IDE for Lisp implementations on any
platform:

http://mclide.in-progress.com/

I am routinely using swank on various lisp implementations, so I am
in a good position to help making swank-clojure consistent with the
normative CL swank server. I'd prefer to work with somebody familiar
with swank-clojure that could review my contributions and integrate
them in the codebase.

-- Terje Norderhaug
te...@in-progress.com

[Terje Norderhaug]

On Feb 13, 2010, at 5:19 PM, wilig wrote:
> I'm willing to give maintenance of swank-clojure a shot. With one
> huge caveat: I'm very new to both clojure and CL, so patience will
> have to be practiced as I slowly get up to speed on a new code base in
> a new language. Unless anyone with more experience wants to jump in,
> I will begin applying patches, and working on open issues.

Your participation is appreciated!

Please consider joining the swank-clojure mailing list:

http://groups.google.com/group/swank-clojure

-- Terje Norderhaug
te...@in-progress.com

[Stuart Halloway]

I had proposed that c.c.sql use c.c.logging, and Steve was ok with
that, but I held off after all the back and forth about logging itself.

I would be just as happy to see no logging & no wrapping/throwing at
this level. If you make a patch that does this (and nag me a
little :-) ) I will make sure it gets discussed on dev and acted on.

Stu

> --
> You received this message because you are subscribed to the Google
> Groups "Clojure" group.
> To post to this group, send email to clo...@googlegroups.com
> Note that posts from new members are moderated - please be patient
> with your first post.
> To unsubscribe from this group, send email to
> clojure+u...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/clojure?hl=en

[Phil Hagelberg]

On Sun, Feb 14, 2010 at 1:34 AM, Steve Purcell <st...@sanityinc.com> wrote:
> I'll bite:
>
>  http://github.com/purcell/swank-clojure
>
> In my master branch (freshly forked from Phil's repo) I've applied Richard's patch, plus the following recent branches from the swank-clojure network on github (http://github.com/technomancy/swank-clojure/network):

Thanks a bunch! I've finally gotten a chance to take a look at these
issues and these changes all seem to make sense.

I've merged and pushed. Will deploy a 1.2.0-SNAPSHOT to clojars so
these changes will soon be usable, but there seem to be some issues
with clojars right now.

-Phil

> bmillare/master:
> * bmillare/master allow start-repl to accept optional arguments to pass to start-server
>
> digash/master:
> * add pprint-eval for slime-pprint-eval-last-expression which is usually bound to C-c C-p
>
> fbreuer/master
> * change path in start-repl for Windows compatibility
>
> hgiddens/customize-project-lib-path:
> * Use `swank-clojure-project-dep-path' in `swank-clojure-project'.
> * Added defcustom form `swank-clojure-project-dep-path'.
> * Documentation updated.
>
>
> I've tested it locally on my OS X machine, and it works fine. In particular, Richard's patch has solved some problems when compiling files that generated reflection warnings.
>
> I can put my swank-clojure jar on Clojars if necessary; just let me know.
>
> If this all works for you, Phil, you can just merge my master branch to slurp in the above changes; I'll refrain from adding anything else to it.
>
> -Steve
>