code review request: clojure.java.io

[Stuart Halloway]

Assembla Ticket #311 [1] calls for the promotion of clojure.contrib.io
into clojure (as clojure.java.io). I have attached a patch, and am
requesting comments and code review from the community.

Reasons you want to take time from your day to read this code:

(1) It's important. This isn't just a copy/paste from contrib, there
are several design changes (documented in the ticket).

(2) It's interesting, demonstrating use of Clojure 1.2 protocols.

(3) If you are new to Clojure, it isn't that scary. It's "just I/O". :-)

Thanks!
Stu

[1] https://www.assembla.com/spaces/clojure/tickets/311-clojure-java-io-namespace

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

[Peter Schuller]

> Assembla Ticket #311 [1] calls for the promotion of clojure.contrib.io into
> clojure (as clojure.java.io). I have attached a patch, and am requesting
> comments and code review from the community.

Should delete-file-recursively recurse down symlinks (on platforms
that support them)? An admittedly Unix biased expected behavior would
be, I believe, to not do that by default, but maybe have an option to
do so.

(Did you want feedback here or on assembla?)

--
/ Peter Schuller

[Stuart Halloway]

Using an option for symlinks raises the possibility of using options
to let delete-file handle everything, e.g.

(delete-file "foo" :recursive true)

instead of

(delete-file-recursively "foo")

Stu

[Michael Harrison (goodmike)]

Stuart,

I think it's a great idea to ask the community for input.

I'm new to working with Assembla (and still a little new to working
with git and github.) So, if I pull down the master branch of Clojure
from http://github.com/richhickey/clojure and apply the patch file I
find on Assembla, I'll have the code to review? It looks like you
modify src/clj/clojure/core.clj a bit, add src/clj/clojure/java/
io.clj, and modify and add testing files. Is this right?

Cheers,
Michael


On May 11, 12:16 pm, Stuart Halloway <stuart.hallo...@gmail.com>
wrote:

> Assembla Ticket #311 [1] calls for the promotion of clojure.contrib.io  
> into clojure (as clojure.java.io). I have attached a patch, and am  
> requesting comments and code review from the community.
>
> Reasons you want to take time from your day to read this code:
>
> (1) It's important. This isn't just a copy/paste from contrib, there  
> are several design changes (documented in the ticket).
>
> (2) It's interesting, demonstrating use of Clojure 1.2 protocols.
>
> (3) If you are new to Clojure, it isn't that scary. It's "just I/O". :-)
>
> Thanks!
> Stu
>

> [1]https://www.assembla.com/spaces/clojure/tickets/311-clojure-java-io-n...

[Stuart Halloway]

Yes, that is right. "git apply" or "git am" should work.

Stu

[Peter Schuller]

> Using an option for symlinks raises the possibility of using options to let
> delete-file handle everything, e.g.
>
> (delete-file "foo" :recursive true)
>
> instead of
>
> (delete-file-recursively "foo")

Yes. A potential argument for not conflating them might be that
"delete-file" is a pretty simple operation with hopefully clear
semantics, while a recursive directory tree operation immediately
opens up several questions. On the one hand, 'rm' sets a precedent
that it is okay. On the other hand, I think most API:s I've come
across do make a special distinction between plain unlinking and more
"elaborate" operations like tree walking removal.

Depending on how far one wants to go, the following may be overkill.
But my thoughts quickly stray towards providing a file tree walker
similar to Python's os.walk(), supporting top-down and bottom-up
traversal, following or not following symlinks and some customizable
error handling behavior. Implementing a 'delete-tree' would then be a
bottom-up walk with a delete action, along the lines of:

(defn delete-tree
[top {:keys [follow-symlinks], :or [false]}]
(doall (for [path (walk-tree top {:direction :bottom-up,
:follow-symlinks follow-symlinks})]
(delete-file path))))

Of course none of this is incompatible with the interface of the
proposed delete-file-recursively (except possibly naming).

If you think it might be suitable for inclusion I'd be interested in
having a stab at it (it's a nicely bite-sized opportunity to write
some clojure).

[Michael Wood]

On 11 May 2010 18:37, Peter Schuller <peter.s...@infidyne.com> wrote:
>> Assembla Ticket #311 [1] calls for the promotion of clojure.contrib.io into
>> clojure (as clojure.java.io). I have attached a patch, and am requesting
>> comments and code review from the community.
>
> Should delete-file-recursively recurse down symlinks (on platforms
> that support them)? An admittedly Unix biased expected behavior would
> be, I believe, to not do that by default, but maybe have an option to
> do so.

Following symlinks in delete-file-recursively sounds like a recipe for
disaster to me.

--
Michael Wood <esio...@gmail.com>

[Laurent PETIT]

Hi Stuart,

Nice piece of job !
I will nonetheless try to be constructive and give you some remarks I
made to myself while reading the patch:

* Shouldn't IOFactory protocol have a docstring to describe the
possible options ? (So that it is possible to extend the protocol even
further, e.g. when working with Eclipse IFile, IResources, etc., and
other "file" I/O APIs as well)
* The 2d line of the docstring of reader function "reads" "The
default implementations of this protocol ...", but reader is not a
protocol, it's a function ...
* same for writer function
* same for input-stream function
* same for output-stream function
* I have bad feelings when I see IOFactory being half the time half
implemented (e.g. for Reader only the reader parts), even if in the
end it's the same thing for users
* I also have bad feelings when I see the Coercions protocol defined
but not used in conjunction with IOFactory for default behaviours
* Decidedly, I have bad feelings when I read about the "magic" of
"coercing" a String first as a URL, and if not possible, fall back and
consider it a local absolute/relative path. I'm "mitigated" in the
sense that either it's too magic and should got rid of, either it's
interesting and could be promoted as a behaviour in the predefined
Coercions ?
* If you define IOFactory on Object, you can then (if I understand
correctly, I didn't play with protocols that much) avoid all the
(assoc default-streams-impl ...) and go with one of the extend-type...
macros and just redefine the overriden parts ? Or is it an (premature
?) optimization ?
* Could it be interesting to have as-reader as-inputstream
as-outputstream as-writer so that the do-copy methods could all be
resolved in terms of those primitive coercions, then resulting in less
handled combinations in the end of the methods definitions ?
* Could as-file be type hinted so that it's not necessary to do the
cast to ^File in as-relative-path ?
* It's not clear from 'file function that it can take any as-file-able
* Couldn't make-parents be renamed create-parents (interestingly
it's the verb "create" that is used in the javadoc)
* It's not clear from make-parents docstring that it accepts same
arguments as 'file function
* After reviewing it again, it's not clear what the function 'file
does in its various arity forms (it can somehow be guessed from the
arg names, but ...)

HTH,

--
Laurent

2010/5/11 Stuart Halloway <stuart....@gmail.com>:

[Peter Schuller]

> Following symlinks in delete-file-recursively sounds like a recipe for
> disaster to me.

Sorry, I should have been more explicit. The (delete-file-recursively
...) in the patch *does* follow symlinks (implicitly, because
isDirectory() returns true for symlinks pointing to diretories).

The simplest "fix" (assuming everyone agrees it's a problem) is to
simply change it not to, but I suggested the option to allow for both
- but the default should presumably be to *not* follow symlinks.

--
/ Peter Schuller

[Michael Wood]

On 11 May 2010 23:16, Peter Schuller <peter.s...@infidyne.com> wrote:
>> Following symlinks in delete-file-recursively sounds like a recipe for
>> disaster to me.
>
> Sorry, I should have been more explicit. The (delete-file-recursively
> ...) in the patch *does* follow symlinks (implicitly, because
> isDirectory() returns true for symlinks pointing to diretories).

Perhaps I should have been more explicit/clear :) Your paragraph
above is what I understood from your original message.

> The simplest "fix" (assuming everyone agrees it's a problem) is to
> simply change it not to, but I suggested the option to allow for both
> - but the default should presumably be to *not* follow symlinks.

I think that treating symlinks to directories as directories when
recursively deleting a directory tree is a bad idea. I am not sure
allowing it as an option is a good idea either, but at least if it's
not the default it's probably OK.

i.e. I agree with you, but am also against allowing this behaviour as
an option, unless others feel it's a good option to have.

--
Michael Wood <esio...@gmail.com>

[Peter Schuller]

> i.e. I agree with you, but am also against allowing this behaviour as
> an option, unless others feel it's a good option to have.

I can definitely buy that.

--
/ Peter Schuller

[Stuart Halloway]

I would like to stop following symlinks, but It doesn't appear that
the File API has any ability to detect symlinks. How were you thinking
about doing this?

[Stefan Bodewig]

On 2010-05-12, Stuart Halloway <stuart....@gmail.com> wrote:

> I would like to stop following symlinks, but It doesn't appear that
> the File API has any ability to detect symlinks.

It sounds as if Java7 will finally support dealing with symlinks via
NIO.2 <http://java.sun.com/developer/technicalArticles/javase/nio/#5>

> How were you thinking about doing this?

The way Ant does it is by comparing the absolute file name with its
canonical name. Actually it's a bit more complex than that, see
isSymbolicLink in
<http://svn.apache.org/repos/asf/ant/core/trunk/src/main/org/apache/tools/ant/util/SymbolicLinkUtils.java>

I should note that getCanonicalPath is a memory and performance hog
(IIUC the Java class library keeps a cache of File -> canonical path).

Stefan

[Peter Schuller]

> I would like to stop following symlinks, but It doesn't appear that the File
> API has any ability to detect symlinks. How were you thinking about doing
> this?

Argh, you are right of course. I somehow believed that *testing* for
whether a path is a symlink was part of File's API, but I failed to
confirm it.

In that case, I suggest putting a huge warning in the documentation at
least, as to the potential destructiveness of the operation.

(Ant's trick with canonical names feels shaky in the context of
something as general-purpose as delete-file)

[Stuart Halloway]

Hi Laurent,

Thanks for the detailed feedback! Responses inline (and updated in a
new patch with #311).

> * Shouldn't IOFactory protocol have a docstring to describe the
> possible options ? (So that it is possible to extend the protocol even
> further, e.g. when working with Eclipse IFile, IResources, etc., and
> other "file" I/O APIs as well)

Yes, added.

> * The 2d line of the docstring of reader function "reads" "The
> default implementations of this protocol ...", but reader is not a
> protocol, it's a function ...
> * same for writer function
> * same for input-stream function
> * same for output-stream function

Yes, added

> * I have bad feelings when I see IOFactory being half the time half
> implemented (e.g. for Reader only the reader parts), even if in the
> end it's the same thing for users

The latter is the point. IOFactory is an abstraction that helps the
implementer, users don't have to worry about it.

> * I also have bad feelings when I see the Coercions protocol defined
> but not used in conjunction with IOFactory for default behaviours

There are very few examples of this, not worth making changing IMO.

> * Decidedly, I have bad feelings when I read about the "magic" of
> "coercing" a String first as a URL, and if not possible, fall back and
> consider it a local absolute/relative path. I'm "mitigated" in the
> sense that either it's too magic and should got rid of, either it's
> interesting and could be promoted as a behaviour in the predefined
> Coercions ?

This is good magic. The space of strings that look like file URLs is
disjoint from those that look like paths, and both are reasonable
expectations (based on my ad hoc survey of other languages.)

Bad magic would be to fall back and interpret the name is a classpath
resource. Some earlier versions did this, and I pulled this into the
separate fn "resource".

> * If you define IOFactory on Object, you can then (if I understand
> correctly, I didn't play with protocols that much) avoid all the
> (assoc default-streams-impl ...) and go with one of the extend-type...
> macros and just redefine the overriden parts ? Or is it an (premature
> ?) optimization ?

No. You cannot define part of a protocol at one level, and then get
the object behavior for missing parts:

> * Could it be interesting to have as-reader as-inputstream
> as-outputstream as-writer so that the do-copy methods could all be
> resolved in terms of those primitive coercions, then resulting in less
> handled combinations in the end of the methods definitions ?

I actually tried that, and it make things worse.

> * Could as-file be type hinted so that it's not necessary to do the
> cast to ^File in as-relative-path ?

This should work but doesn't yet, protocol type hint handling needs to
flow through.

> * It's not clear from 'file function that it can take any as-file-
> able

Docstring improved.

> * Couldn't make-parents be renamed create-parents (interestingly
> it's the verb "create" that is used in the javadoc)

It's the original name from contrib. I am reluctant to change a name
people have gotten used to unless the other choice is overwhelmingly
better. I'll listen to the community on this one.

> * It's not clear from make-parents docstring that it accepts same
> arguments as 'file function

I like the docstring as-is but will take a better one if given it.

> * After reviewing it again, it's not clear what the function 'file
> does in its various arity forms (it can somehow be guessed from the
> arg names, but ...)

Improved.

Thanks!
Stu

[Michael Wood]

Hi

On 13 May 2010 03:02, Stuart Halloway <stuart....@gmail.com> wrote:
>>  * Decidedly, I have bad feelings when I read about the "magic" of
>> "coercing" a String first as a URL, and if not possible, fall back and
>> consider it a local absolute/relative path. I'm "mitigated" in the
>> sense that either it's too magic and should got rid of, either it's
>> interesting and could be promoted as a behaviour in the predefined
>> Coercions ?
>
> This is good magic. The space of strings that look like file URLs is
> disjoint from those that look like paths, and both are reasonable
> expectations (based on my ad hoc survey of other languages.)

They are actually not disjoint:

$ mkdir -p "http://www.example.org/cgi-bin/"
$ echo something >"http://www.example.org/something.cgi?key=value&fred=bloggs"
$ echo blah >"http://www.example.org/cgi-bin/test.cgi?x=y&full-name=Fred%20Bloggs"
$ find http\://
http://
http://www.example.org
http://www.example.org/something.cgi?key=value&fred=bloggs
http://www.example.org/cgi-bin
http://www.example.org/cgi-bin/test.cgi?x=y&full-name=Fred%20Bloggs
$ cat "http://www.example.org/cgi-bin/test.cgi?x=y&full-name=Fred%20Bloggs"
blah

And the same would, of course, work for file:/// instead of http://.

Of course it's unusual to do something like the above, but there's
nothing wrong with those paths. Unix filesystems have no problem with
":", "?", "=", "&", etc. in file or directory names and multiple
directory separators are treated as if there was just one.

--
Michael Wood <esio...@gmail.com>

[Laurent PETIT]

2010/5/13 Stuart Halloway <stuart....@gmail.com>:

>>  * I also have bad feelings when I see the Coercions protocol defined
>> but not used in conjunction with IOFactory for default behaviours
>
> There are very few examples of this, not worth making changing IMO.

I'll try to do this as an exercise if time permits. I don't like not
respecting DRY principle just after having defined the required
functions ...

> No. You cannot define part of a protocol at one level, and then get the
> object behavior for missing parts:
>
>>  * Could it be interesting to have as-reader as-inputstream
>> as-outputstream as-writer so that the do-copy methods could all be
>> resolved in terms of those primitive coercions, then resulting in less
>> handled combinations in the end of the methods definitions ?
>
> I actually tried that, and it make things worse.

Worse in what sense ? More locs ? Less readable ? (Sometimes one needs
to have more lines of code for less duplication of the logic (more
lines of code resulting in more functions being defined, with their
ceremonies, even if those are very short in clojure), and I don't
necessarily consider this a bad thing).

>>  * Could as-file be type hinted so that it's not necessary to do the
>> cast to ^File in as-relative-path ?
>
> This should work but doesn't yet, protocol type hint handling needs to flow
> through.

Is it expected to work before beta1 ?

>>  * Couldn't make-parents be renamed create-parents (interestingly
>> it's the verb "create" that is used in the javadoc)
>
> It's the original name from contrib. I am reluctant to change a name people
> have gotten used to unless the other choice is overwhelmingly better. I'll
> listen to the community on this one.

You're right. And "mkdir" is derived from "make dir", after all.

>>  * It's not clear from make-parents docstring that it accepts same
>> arguments as 'file function
>
> I like the docstring as-is but will take a better one if given it.

What about aligning its args definition as well as its docstring with
those of function file, as in :

(defn make-parents
"Pass arg(s) as you would to function file, create all parent
directories of the file they represent. "
{:added "1.2"}
[arg & more]
(.mkdirs (.getParentFile ^File (apply file arg more))))

[Stuart Halloway]

I'm comfortable with the behavior implied for these corner cases.

Stu

[Michael Wood]

On 14 May 2010 04:32, Stuart Halloway <stuart....@gmail.com> wrote:
>> On 13 May 2010 03:02, Stuart Halloway <stuart....@gmail.com> wrote:
>>>>
>>>>  * Decidedly, I have bad feelings when I read about the "magic" of
>>>> "coercing" a String first as a URL, and if not possible, fall back and
>>>> consider it a local absolute/relative path. I'm "mitigated" in the
>>>> sense that either it's too magic and should got rid of, either it's
>>>> interesting and could be promoted as a behaviour in the predefined
>>>> Coercions ?
>>>
>>> This is good magic. The space of strings that look like file URLs is
>>> disjoint from those that look like paths, and both are reasonable
>>> expectations (based on my ad hoc survey of other languages.)
>>
>> They are actually not disjoint:
>>
>> $ mkdir -p "http://www.example.org/cgi-bin/"

[...]

> I'm comfortable with the behavior implied for these corner cases.

So if program runs from a particular directory and references files as
file:///some/file, then if someone can create a directory called file:
in that directory with some/file inside that, the program will
suddenly try to access the wrong thing? Seems suspicious to me.

[Laurent PETIT]

2010/5/14 Michael Wood <esio...@gmail.com>:

> On 14 May 2010 04:32, Stuart Halloway <stuart....@gmail.com> wrote:
>>> On 13 May 2010 03:02, Stuart Halloway <stuart....@gmail.com> wrote:
>>>>>
>>>>>  * Decidedly, I have bad feelings when I read about the "magic" of
>>>>> "coercing" a String first as a URL, and if not possible, fall back and
>>>>> consider it a local absolute/relative path. I'm "mitigated" in the
>>>>> sense that either it's too magic and should got rid of, either it's
>>>>> interesting and could be promoted as a behaviour in the predefined
>>>>> Coercions ?
>>>>
>>>> This is good magic. The space of strings that look like file URLs is
>>>> disjoint from those that look like paths, and both are reasonable
>>>> expectations (based on my ad hoc survey of other languages.)
>>>
>>> They are actually not disjoint:
>>>
>>> $ mkdir -p "http://www.example.org/cgi-bin/"
> [...]
>> I'm comfortable with the behavior implied for these corner cases.
>
> So if program runs from a particular directory and references files as
> file:///some/file, then if someone can create a directory called file:
> in that directory with some/file inside that, the program will
> suddenly try to access the wrong thing?  Seems suspicious to me.
>

Michael has a point. One could write a cache mechanism for accessing
urls and should work around this "white" (??) magic ...

[Stuart Halloway]

> So if program runs from a particular directory and references files as
> file:///some/file, then if someone can create a directory called file:
> in that directory with some/file inside that, the program will
> suddenly try to access the wrong thing? Seems suspicious to me.

Two points:

(1) This is not what the code does. If it looks like a URL, it will be
treated like a URL, period. The io fns will simply refuse to open a
file whose name looks like a URL.

(2) The purpose of the IO helper fns is not to provide the "one true
way" to get things. Their purpose is convenience. There are *already*
easy ways to get things, and to be exact about interpretation. These
even play nice with the io functions:

; convenient
(reader "blah")

; URL or die
(reader (URL. "blah"))

; File or die
(reader (File. "blah"))

Stu

[Michael Wood]

On 14 May 2010 13:50, Stuart Halloway <stuart....@gmail.com> wrote:
>> So if program runs from a particular directory and references files as
>> file:///some/file, then if someone can create a directory called file:
>> in that directory with some/file inside that, the program will
>> suddenly try to access the wrong thing?  Seems suspicious to me.
>
> Two points:
>
> (1) This is not what the code does. If it looks like a URL, it will be
> treated like a URL, period.  The io fns will simply refuse to open a file
> whose name looks like a URL.

OK, thanks for clarifying. Sorry for my confusion :)

> (2) The purpose of the IO helper fns is not to provide the "one true way" to
> get things. Their purpose is convenience. There are *already* easy ways to
> get things, and to be exact about interpretation. These even play nice with
> the io functions:
>
> ; convenient
> (reader "blah")
>
> ; URL or die
> (reader (URL. "blah"))
>
> ; File or die
> (reader (File. "blah"))

OK, then I retract my complaints and thanks for these examples.

--
Michael Wood <esio...@gmail.com>

[Rasmus Svensson]

2010/5/11 Stuart Halloway <stuart....@gmail.com>

Assembla Ticket #311 [1] calls for the promotion of clojure.contrib.io into clojure (as clojure.java.io). I have attached a patch, and am requesting comments and code review from the community.

I think I have found a bug in the clojure.contrib.io code.

The current 1.2 master branch contains the following :

  (extend Socket
    IOFactory
    (assoc default-streams-impl
      :make-input-stream (fn [^Socket x opts] (.getInputStream x))
      :output-stream (fn [^Socket x opts] (output-stream (.getOutputStream x) opts))))

Note that :output-stream is not part of the IOFactory protocol. The intended keyword was most probably :make-output-stream. Also, the body should not contain a call to output-stream, since it calls make-output-stream. I propose that the last line is changed so that the code becomes the following:

  (extend Socket
    IOFactory
    (assoc default-streams-impl
      :make-input-stream (fn [^Socket x opts] (.getInputStream x))
      :make-output-stream (fn [^Socket x opts] (.getOutputStream x))))

Now, one can make output streams from sockets again (this threw an exception[1] before):

  (output-stream (Socket. "example.com" 1234))

So what is the next step for me now? I am not formally a contributor yet. It will take approx a week until my signed CA arrives. I will gladly help with whatever I can do in the mean time...

// raek

[1] The exception: java.lang.IllegalArgumentException: Cannot open <#<Socket Socket[addr=clojure.org/75.126.104.177,port=80,localport=50897]>> as an OutputStream.

[Chas Emerick]

Sorry I'm so late to this particular discussion :-/

Why was to-byte-array left behind? It is a bit of an oddball
function, insofar as it doesn't fit into IOFactory or Coercions, but
seems to me like a natural cousin to copy.

- Chas

[Stuart Halloway]

No one spoke up for it.

> Sorry I'm so late to this particular discussion :-/
>
> Why was to-byte-array left behind? It is a bit of an oddball function, insofar as it doesn't fit into IOFactory or Coercions, but seems to me like a natural cousin to copy.
>
> - Chas
>
> On May 11, 2010, at 12:16 PM, Stuart Halloway wrote:
>
>> Assembla Ticket #311 [1] calls for the promotion of clojure.contrib.io into clojure (as clojure.java.io). I have attached a patch, and am requesting comments and code review from the community.
>>
>> Reasons you want to take time from your day to read this code:
>>
>> (1) It's important. This isn't just a copy/paste from contrib, there are several design changes (documented in the ticket).
>>
>> (2) It's interesting, demonstrating use of Clojure 1.2 protocols.
>>

>> (3) If you are new to Clojure, it isn't that scary. It's "just I/O". :-)