Clojure

into loses metadata

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.3
  • Fix Version/s: Release 1.5
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

The into fn loses metadata.

I'm seeing some team members (myself included) rely on metadata. Metadata has been incredibly useful in some cases, however the silent destruction of metadata by core clojure fns (into, walk, etc) have been a source of increasing complexity and confusion.

A team member could start relying on a 3rd party library that used 'into'. Actually, the into fn could essentially be added to the code base at any time in many ways. Wrt metadata the potential for incidental complexity increases exponentially as more developers and libraries enter the mix.

One of the reasons Clojure has worked for us so well is because it has greatly reduced incidental complexity.
IMHO the 'into metadata bug' seriously limits the usefulness of metadata and would love to see the into fn fixed in 1.4.

Activity

Hide
Andy Fingerhut added a comment -

Someone please correct this if it is wrong, but it seems that into loses metadata because it is implemented using transients, and transients lose metadata. If true, then one way to fix this ticket is to implement metadata for transients.

Are there any fundamental reasons why it would be difficult to add metadata to transients, as long as the metadata itself was immutable?

Show
Andy Fingerhut added a comment - Someone please correct this if it is wrong, but it seems that into loses metadata because it is implemented using transients, and transients lose metadata. If true, then one way to fix this ticket is to implement metadata for transients. Are there any fundamental reasons why it would be difficult to add metadata to transients, as long as the metadata itself was immutable?
Hide
Andy Fingerhut added a comment - - edited

First rough cut of a patch that makes not only into, but also select-keys, clojure.set/project, and clojure.set/rename preserve metadata. Added tests for these and many other functions. Still some open questions about other functions not tested in comments. This patch does not attempt to make transients preserve metadata.

Show
Andy Fingerhut added a comment - - edited First rough cut of a patch that makes not only into, but also select-keys, clojure.set/project, and clojure.set/rename preserve metadata. Added tests for these and many other functions. Still some open questions about other functions not tested in comments. This patch does not attempt to make transients preserve metadata.
Hide
Andy Fingerhut added a comment - - edited

clj-916-make-into-and-others-preserve-metadata-patch2.txt is semantically same patch and comments as March 16, 2012. Merely touched up to apply cleanly to latest master as of Mar 23, 2012.

Show
Andy Fingerhut added a comment - - edited clj-916-make-into-and-others-preserve-metadata-patch2.txt is semantically same patch and comments as March 16, 2012. Merely touched up to apply cleanly to latest master as of Mar 23, 2012.
Hide
Aaron Brooks added a comment -

I just ran into this issue myself. Is there anything that I could do to help get this fixed? Test writing? Patch checks? I'm happy to help in any way I can.

Show
Aaron Brooks added a comment - I just ran into this issue myself. Is there anything that I could do to help get this fixed? Test writing? Patch checks? I'm happy to help in any way I can.
Hide
Andy Fingerhut added a comment -

You could examine the existing patch, including its tests, and see if it would have done what you were hoping it would do. Add a comment here regarding whether the changes look good to you. The attached patch is already on my weekly list of prescreened patches, but it is only one among many.

Show
Andy Fingerhut added a comment - You could examine the existing patch, including its tests, and see if it would have done what you were hoping it would do. Add a comment here regarding whether the changes look good to you. The attached patch is already on my weekly list of prescreened patches, but it is only one among many.
Hide
Fogus added a comment -

The code is clean and tests test what they should test. Regarding the questions in the test file:

  • I recommend (royal) we create another ticket for the remaining clojure.set functions and discuss them there. Once resolved the questions in the metadata.clj test file should be removed.
  • remove returns a LazySeq, but the intent of the question is clear. I vote that remove preserve metadata as well, but that should be the topic of yet another ticket.

One final point. I'd prefer that tickets be addressed in isolation and not contain extra fixes. This is a personal preference, but one that I'd like to take up on the clojure-dev list.

Show
Fogus added a comment - The code is clean and tests test what they should test. Regarding the questions in the test file:
  • I recommend (royal) we create another ticket for the remaining clojure.set functions and discuss them there. Once resolved the questions in the metadata.clj test file should be removed.
  • remove returns a LazySeq, but the intent of the question is clear. I vote that remove preserve metadata as well, but that should be the topic of yet another ticket.
One final point. I'd prefer that tickets be addressed in isolation and not contain extra fixes. This is a personal preference, but one that I'd like to take up on the clojure-dev list.
Hide
Andy Fingerhut added a comment -

clj-916-make-into-preserve-metadata-patch1.txt dated Aug 15 2012 only changes the behavior of into so that it preserves metadata. One or more other tickets will be created for some other functions that currently do not preserve metadata, but perhaps should.

Show
Andy Fingerhut added a comment - clj-916-make-into-preserve-metadata-patch1.txt dated Aug 15 2012 only changes the behavior of into so that it preserves metadata. One or more other tickets will be created for some other functions that currently do not preserve metadata, but perhaps should.

People

Vote (4)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: