<< Back to previous view

[CLJ-960] Capture :column metadata (needed for ClojureScript source maps) Created: 27/Mar/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.5

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: patch

Attachments: File CLJ-960-tests.diff     Text File columns-1.patch     Text File columns-1-v2.patch    
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



 Comments   
Comment by David Nolen [ 13/Apr/12 8:29 AM ]

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

Comment by Brandon Bloom [ 31/May/12 6:09 PM ]

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.

Comment by Andy Fingerhut [ 31/May/12 6:32 PM ]

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.

Comment by John Szakmeister [ 27/Jul/12 5:32 AM ]

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.

Comment by Brandon Bloom [ 27/Jul/12 7:33 PM ]

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?

Comment by David Nolen [ 27/Jul/12 7:36 PM ]

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.

Comment by Brandon Bloom [ 25/Aug/12 9:38 PM ]

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

Comment by Rich Hickey [ 04/Oct/12 8:51 AM ]

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

Comment by Chas Emerick [ 19/Oct/12 11:45 AM ]

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

Comment by Stuart Halloway [ 19/Oct/12 3:02 PM ]

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.

Generated at Fri Oct 24 07:19:29 CDT 2014 using JIRA 4.4#649-r158309.