Clojure

protocol-ize with-open

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Triaged

Description

Good use (and documentation example) of protocols: make with-open aware of a Closable protocol for APIs that use a different close convention. See http://groups.google.com/group/clojure/browse_thread/thread/86c87e1fc4b1347c

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Assembla Importer added a comment -

Converted from http://www.assembla.com/spaces/clojure/tickets/308

Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/308
Hide
Permalink
Tassilo Horn added a comment -

Added a CloseableResource protocol and extended it on java.io.Closeable (implemented by all Readers, Writers, Streams, Channels, Sockets). Use it in with-open.

All tests pass.

Show
Tassilo Horn added a comment - Added a CloseableResource protocol and extended it on java.io.Closeable (implemented by all Readers, Writers, Streams, Channels, Sockets). Use it in with-open. All tests pass.
Tassilo Horn made changes -
Field Original Value New Value
Attachment 0001-Implement-CLJ-308-Added-a-CloseableResource-protocol.patch [ 10754 ]
Hide
Permalink
Tassilo Horn added a comment -

Seems to be related to Scopes (http://dev.clojure.org/jira/browse/CLJ-2).

Show
Tassilo Horn added a comment - Seems to be related to Scopes (http://dev.clojure.org/jira/browse/CLJ-2).
Hide
Permalink
Tassilo Horn added a comment -

Updated patch.

Show
Tassilo Horn added a comment - Updated patch.
Tassilo Horn made changes -
Attachment 0001-Added-ClosableResource-protocol-for-with-open.patch [ 10986 ]
Tassilo Horn made changes -
Attachment 0001-Implement-CLJ-308-Added-a-CloseableResource-protocol.patch [ 10754 ]
Andy Fingerhut made changes -
Priority Blocker [ 1 ]
Patch Code [ 10001 ]
Hide
Permalink
Andy Fingerhut added a comment -

Patch 0001-Added-ClosableResource-protocol-for-with-open.patch dated 08/Mar/12 applies, builds, and tests cleanly on latest master as of Apr 2 2012. Tassilo has signed a CA.

Show
Andy Fingerhut added a comment - Patch 0001-Added-ClosableResource-protocol-for-with-open.patch dated 08/Mar/12 applies, builds, and tests cleanly on latest master as of Apr 2 2012. Tassilo has signed a CA.
Hide
Permalink
Tassilo Horn added a comment -

Updated patch to apply cleanly against master again.

Show
Tassilo Horn added a comment - Updated patch to apply cleanly against master again.
Tassilo Horn made changes -
Attachment 0001-Added-ClosableResource-protocol-for-with-open.patch [ 11047 ]
Andy Fingerhut made changes -
Priority Blocker [ 1 ] Minor [ 4 ]
Tassilo Horn made changes -
Attachment 0001-Added-ClosableResource-protocol-for-with-open.patch [ 10986 ]
Alex Miller made changes -
Fix Version/s Backlog [ 10035 ]
Alex Miller made changes -
Labels io
Alex Miller made changes -
Reporter Assembla Importer [ importer ]
Approval Triaged [ 10120 ]
Hide
Permalink
Brandon Bloom added a comment -

I looked up this ticket because I ran in to a reflection warning: with-open does not hint it's binding with java.io.Closeable

Some feedback on the patch:

1) This is a breaking change for anyone relying on the close method to be duck-typed.

2) CloseableResource is a bit long. clojure.core.protocols.Closeable is plenty unambiguous.

3) Rather than extending CloseableResource to java.io.Closeable, you can use the little known (undocumented? unsupported?) :on-interface directive:

(defprotocol Closeable
  :on-interface java.io.Closeable
  (close [this]))

That would perform much better than the existing patch.

Show
Brandon Bloom added a comment - I looked up this ticket because I ran in to a reflection warning: with-open does not hint it's binding with java.io.Closeable Some feedback on the patch: 1) This is a breaking change for anyone relying on the close method to be duck-typed. 2) CloseableResource is a bit long. clojure.core.protocols.Closeable is plenty unambiguous. 3) Rather than extending CloseableResource to java.io.Closeable, you can use the little known (undocumented? unsupported?) :on-interface directive: 
(defprotocol Closeable
  :on-interface java.io.Closeable
  (close [this]))
That would perform much better than the existing patch.
Hide
Permalink
Tassilo Horn added a comment -

Hi Brandon, two questions:

Could 1) be circumvented somehow by providing a default implementation somehow? I guess the protocol could be extended upon Object with implementation (.close this), but that would give a reflection warning since Object has no close method. Probably one could extend upon Object and in the implementation search a "close" method using java.lang.reflect and throw an exception if none could be found?

Could you please tell me a bit more about the :on-interface option? How does that differ from extend? And how do I add the implementation, i.e., (.close this) with that option?

Show
Tassilo Horn added a comment - Hi Brandon, two questions: Could 1) be circumvented somehow by providing a default implementation somehow? I guess the protocol could be extended upon Object with implementation (.close this), but that would give a reflection warning since Object has no close method. Probably one could extend upon Object and in the implementation search a "close" method using java.lang.reflect and throw an exception if none could be found? Could you please tell me a bit more about the :on-interface option? How does that differ from extend? And how do I add the implementation, i.e., (.close this) with that option?
Alex Miller made changes -
Priority Minor [ 4 ] Major [ 3 ]
Alex Miller made changes -
Priority Major [ 3 ] Critical [ 2 ]
Alex Miller made changes -
Priority Critical [ 2 ] Major [ 3 ]

People

Vote (15)
Watch (8)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.