Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Update clojure-ts-find-ns to supportin-ns and Add tests#38

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bbatsov merged 5 commits intoclojure-emacs:mainfromp4v4n:find-ns-in-ns
Feb 19, 2024

Conversation

@p4v4n
Copy link
Contributor

  • Add support forin-ns when finding namespace.
  • Remove text property of ns.
  • Add a couple of test helpers and find-ns tests equivalent toclojure-mode.

(with-clojure-ts-buffer"(in-ns 'bar.baz)"
(expect (clojure-ts-find-ns):to-equal"bar.baz")))

(it"should take the first ns instead of closest unlike clojure-mode"
Copy link
ContributorAuthor

@p4v4np4v4nFeb 19, 2024
edited
Loading

Choose a reason for hiding this comment

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

This is the only scenario where it differs from clojure-find-ns after this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason for this change?

Copy link
ContributorAuthor

@p4v4np4v4nFeb 19, 2024
edited
Loading

Choose a reason for hiding this comment

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

AFAIU It's debatable what is the correct behaviour here.
clojure-mode finds the closest ns above point and falls back to finding the closest ns below point.

  1. My understanding is that clojure-mode finding the closest ns is more of a result of an implementation detail rather than desired behaviour.

  2. Also I haven't figured out yet how to find all occurrences or do a reverse search with tree-sitter queries.

Copy link
Member

Choose a reason for hiding this comment

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

I recall in the beginningclojure-mode was just looking for ans form in the beginning of the file, but then we changed it, as per the Clojure specification the currentns was supposed to be whatever ns is closest above the form in question. I don't recall doing a search below as well, but perhaps there was some reason for this.

Copy link
Member

Choose a reason for hiding this comment

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

Anyways, if by first you mean "closest above" (and so it seems looking at the tests), I think that's fine.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Right now it's first from the beginning of file. (point is not used at all).
This is the behaviour before the PR as well. I haven't updated it as it would require more complicated tree-sitter queries.

The below test is matching the 3rd ns only because the first 2 are invalid not because of the point.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

If clojure-mode behaviour is ideal I would prefer to create a new issue and handle it in a separate PR.(Need more tree-sitter knowledge to handle the queries there.)

bbatsov reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

That'd be fine by me.

Copy link
Member

Choose a reason for hiding this comment

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

In general that has never been super important for me (to do this by the letter), as usually source files have only the leadingns form. But clearly it was a problem for some users ofclojure-mode. :D

p4v4n reacted with thumbs up emoji

Choose a reason for hiding this comment

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

@p4v4n If you'd like to chat about tree-sitter-clojure, there is a gitter / matrix channel.

See the bottom ofthis for links if interested.

p4v4n reacted with thumbs up emoji
@@ -0,0 +1,50 @@
;;; test-helper.el --- Clojure TS Mode: Non-interactive unit-test setup -*-lexical-binding:t; -*-

;; Copyright © 2022-2024 Danny Freeman
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to preserve the copyright attribution of the original code.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Will add them back.

@bbatsovbbatsov merged commit0fa4944 intoclojure-emacs:mainFeb 19, 2024
@p4v4np4v4n deleted the find-ns-in-ns branchFebruary 19, 2024 20:12
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@bbatsovbbatsovbbatsov left review comments

+1 more reviewer

@sogaiusogaiusogaiu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@p4v4n@bbatsov@sogaiu

[8]ページ先頭

©2009-2025 Movatter.jp