Clojure

Capture :column metadata (needed for ClojureScript source maps)

Details

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

Description

I've begun working on implementing SourceMaps for ClojureScript. For an overview of SourceMaps, see: http://www.html5rocks.com/en/tutorials/developertools/sourcemaps/ For discussion of the feature in ClojureScript, see: https://groups.google.com/d/topic/clojure-dev/zgmXO2iM1JQ/discussion

In order to produce accurate source maps, I need column information in addition to line information from the Clojure reader.

I've made the necessary enhancement to LispReader, etc. but have some cleanup and testing left to do. I'd also like a sanity check from the core team before attaching a formal patch. You can find my work in progress here: https://github.com/brandonbloom/clojure/compare/columns

Attachments

  1. File
    CLJ-960-tests.diff
    19/Oct/12 11:45 AM
    2 kB
    Chas Emerick
  2. Text File
    columns-1.patch
    14/Apr/12 6:43 PM
    40 kB
    Brandon Bloom
  3. Text File
    columns-1-v2.patch
    27/Jul/12 5:32 AM
    40 kB
    John Szakmeister

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
David Nolen added a comment -

You need to attach a patch so this can be assessed. Thanks!

Show
David Nolen added a comment - You need to attach a patch so this can be assessed. Thanks!
Rich Hickey made changes -
Field Original Value New Value
Labels patch,
Brandon Bloom made changes -
Attachment columns-1.patch [ 11050 ]
Brandon Bloom made changes -
Labels patch patch,
Hide
Permalink
Brandon Bloom added a comment -

Sorry David! I added a patch a while ago but forgot to add the patch label(s?) as well as leave a comment.

The patch may be out of date now, but I haven't checked. Hopefully the prescreening process will pick it up automatically run the tests.

Show
Brandon Bloom added a comment - Sorry David! I added a patch a while ago but forgot to add the patch label(s?) as well as leave a comment. The patch may be out of date now, but I haven't checked. Hopefully the prescreening process will pick it up automatically run the tests.
Hide
Permalink
Andy Fingerhut added a comment -

Yes, Brandon, your patch has been on the list of prescreened patches since April 15. It has always applied and built cleanly during that time. That patch label is nice to have for certain JIRA report filters, but isn't necessary for the prescreening process to pick it up.

Show
Andy Fingerhut added a comment - Yes, Brandon, your patch has been on the list of prescreened patches since April 15. It has always applied and built cleanly during that time. That patch label is nice to have for certain JIRA report filters, but isn't necessary for the prescreening process to pick it up.
Hide
Permalink
John Szakmeister added a comment -

v2 now applies against the current master (191b05f1). The original patch seemed to be broken from a whitespace perspective, which was making it difficult to apply--even in a 3-way merge. The only real conflict was in Compiler.java where a "final int line" was added to CompilerException. All the tests passed.

Show
John Szakmeister added a comment - v2 now applies against the current master (191b05f1). The original patch seemed to be broken from a whitespace perspective, which was making it difficult to apply--even in a 3-way merge. The only real conflict was in Compiler.java where a "final int line" was added to CompilerException. All the tests passed.
John Szakmeister made changes -
Attachment columns-1-v2.patch [ 11401 ]
Hide
Permalink
Brandon Bloom added a comment -

It looks like the line field was added to CompilerException in commit 89245c68, but that commit doesn't use it for anything. Maybe a later commit uses it? Also, if we want to keep that field, can we also add a column field for patch v3?

Show
Brandon Bloom added a comment - It looks like the line field was added to CompilerException in commit 89245c68, but that commit doesn't use it for anything. Maybe a later commit uses it? Also, if we want to keep that field, can we also add a column field for patch v3?
Hide
Permalink
David Nolen added a comment -

This patch is an enhancement. In order for this one to make any movement I believe it will need a design page outlining the problem, what this solves / alternatives.

Show
David Nolen added a comment - This patch is an enhancement. In order for this one to make any movement I believe it will need a design page outlining the problem, what this solves / alternatives.
Hide
Permalink
Brandon Bloom added a comment -

http://dev.clojure.org/display/design/Compiler+tracking+of+columns

I added a design page, but it seems like overkill. This is a straightforward enhancement...

Show
Brandon Bloom added a comment - http://dev.clojure.org/display/design/Compiler+tracking+of+columns I added a design page, but it seems like overkill. This is a straightforward enhancement...
Hide
Permalink
Rich Hickey added a comment -

I've applied the v2 patch (thanks!), but before we close the ticket can we please get a patch comprising some tests?

Show
Rich Hickey added a comment - I've applied the v2 patch (thanks!), but before we close the ticket can we please get a patch comprising some tests?
Rich Hickey made changes -
Approval Vetted [ 10003 ]
Fix Version/s Release 1.5 [ 10150 ]
Labels patch patch, patch
Hide
Permalink
Chas Emerick added a comment - - edited

Attached CLJ-960-tests.diff to verify :line and :column metadata as now provided by LineNumberingPushbackReader.

Show
Chas Emerick added a comment - - edited Attached CLJ-960-tests.diff to verify :line and :column metadata as now provided by LineNumberingPushbackReader.
Chas Emerick made changes -
Attachment CLJ-960-tests.diff [ 11579 ]
Hide
Permalink
Stuart Halloway added a comment -

The tests are a little fragile (exact map comparison) and so will break if the reader ever does more things. In library and app projects I write tests like this using core.match, but that isn't available as a build dependency here.

Marking screened anyway.

Show
Stuart Halloway added a comment - The tests are a little fragile (exact map comparison) and so will break if the reader ever does more things. In library and app projects I write tests like this using core.match, but that isn't available as a build dependency here. Marking screened anyway.
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (2)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved:
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.