Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect forms :line and :column meta when provided #210

Closed

Conversation

jpmonettas
Copy link

There is currently a difference between Clojure and ClojureScript in how the readers handle the provided :line and :column meta for forms.

In Clojure:

Clojure 1.11.1
user=> ^{:line 40} (defn foo [])
#'user/foo
user=> (-> #'foo meta :line)
40

while in ClojureScript:

ClojureScript 1.11.60
cljs.user=> ^{:line 40} (defn foo [])
#'cljs.user/foo
cljs.user=> (-> #'foo meta :line)
1

This patch just gives precedence to the lines and columns provided by the user which allows tooling that are sending forms to the repl to set the correct line and column for the forms if they know them. This is specially important in ClojureScript where there isn't a way of modifying vars meta.

Since ClojureScript is distributing a vendored tools.reader probably this is a patch that should be submitted to the tools.reader project but trying here first to get directions.

@swannodette
Copy link
Member

@jpmonettas Heya sorry I don't really follow PRs here. They exist almost entirely for me to verify that a patch or some other work isn't going to break everything. In anycase please see https://github.com/clojure/clojurescript/blob/master/script/vendorize_deps, we really don't want to change the vendorize stuff by hand. The right way is to patch the dependency repo, cut a release, rerun the vendorize script, and commit the result. Thanks and sorry for the slowness.

@jpmonettas
Copy link
Author

@swannodette Hey np, I imagined PRs here weren't being used and also created the ask Clojure entry https://ask.clojure.org/index.php/13085/clojurescript-doesnt-respect-provided-line-column-functions. Where is the correct place to report this kind of things?
Anyways, thanks for your work and I will push this to tools.reader instead.

@swannodette
Copy link
Member

@jpmonettas did this get fixed in tools reader? If so then just open an issue in ClojureScript JIRA. We have a vendorize script and we can re-run those if we need to update. If this seems sufficient to then I think we can close this PR.

@jpmonettas
Copy link
Author

@swannodette yes! it was merged here clojure/tools.reader@88d47e6

@jpmonettas jpmonettas closed this Nov 16, 2023
@swannodette
Copy link
Member

@jpmonettas if you want to open a ticket in JIRA that requests to update tools.reader that would be great.

@jpmonettas
Copy link
Author

@swannodette I tried but couldn't. It says my account doesn't have access there and I can't create the ticket without the login.
latest-screenshot

@swannodette
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants