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

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

Activity

Hide
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.
Hide
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.
Hide
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?
Hide
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
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
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
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.
Hide
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
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
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!

People

Vote (2)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: