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

Pass nREPL client evaled code's line and col nums to the reader #1038

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Sep 4, 2024

Hi,

could you please consider patch to pass the nREPL client's eval file line and column numbers down to the reader for more accurate compiler exception reporting. It fixes #1037.

I have attempted a focused approach to propagate the line and column parameters to the error reporting mechanism. If this approach is not optimal, I am open to reorganizing the code based on better suggestions.

I've included tests down the affected path to cover these changes.

Thanks

Copy link
Member

@chrisrink10 chrisrink10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. My main comments are just around the naming of the kwargs and a few stylistic things with the documentation.

src/basilisp/lang/reader.py Outdated Show resolved Hide resolved
src/basilisp/core.lpy Outdated Show resolved Hide resolved
src/basilisp/core.lpy Outdated Show resolved Hide resolved
tests/basilisp/contrib/nrepl_server_test.lpy Outdated Show resolved Hide resolved
src/basilisp/core.lpy Outdated Show resolved Hide resolved
Comment on lines 578 to 580
(client-send! client {:id (id-inc!) :ns "abc.xyz":op "eval" :code "(abc)"
:file filename
:line 6 :column 2})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(client-send! client {:id (id-inc!) :ns "abc.xyz":op "eval" :code "(abc)"
:file filename
:line 6 :column 2})
(client-send! client {:id (id-inc!) :ns "abc.xyz" :op "eval" :code "(abc)"
:file filename
:line 6 :column 2})

I guess I wouldn't hate if this was each key on its own line, but if not then can you at least add a space between this string and keyword.

Copy link
Contributor Author

@ikappaki ikappaki Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to add a space. Apologies for not placing each key on its own line; the rest of the test file follows an ad-hoc style for (my) visual clarity and space conservation, which might seem inconsistent otherwise. Perhaps we could look into a tool similar to black in Python for automatic code formatting?

One tool that comes to mind is cljfmt, which could have caught this with its :insert-missing-whitespace? option and even better would have split the keys into their own lines using :split-keypairs-over-multiple-lines?.

Copy link
Member

@chrisrink10 chrisrink10 Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea and as you know I've configured formatting checks for the Python code. However, I recently surveyed the Clojure formatting tool space for just this purpose and found myself unsatisfied with any of the options. I primarily write Clojure in Emacs (Spacemacs) and have always found that I prefer its implicit formatting style (with alignment), but none of the tools would enforce that style for me without other drawbacks or difficulties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea and as you know I've configured formatting checks for the Python code.

Indeed, this is why I mentioned it. It's very useful.

I primarily write Clojure in Emacs (Spacemacs) and have always found that I prefer its implicit formatting style (with alignment), but none of the tools would enforce that style for me without other drawbacks or difficulties.

Out of curiosity, which specific alignment setting do you find missing in cljfmt? It seems fairly configurable, or is it more the overall alignment style that’s tricky to set up?

@chrisrink10 chrisrink10 merged commit c1e67c2 into basilisp-lang:main Sep 5, 2024
12 checks passed
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.

nREPL server: Incorrect Line Number in Compiler Exception
2 participants