Skip to content

Conversation

@p4v4n
Copy link
Contributor

@p4v4n p4v4n commented Sep 7, 2023

Closes #656
Follow-up for #661

  • Have initially tried using a regex and syntax table first to detect if namespace is inside metadata but it was too difficult.
  • This change re-frames the problem a little. Instead of checking if the namespace is inside string,comment, metadata e.t.c it checks if the current ns form is top-level form.
  • Copied most of the test cases from @vemv previous commit.
@p4v4n p4v4n force-pushed the fix-clojure-find-ns-part2 branch from 6655971 to ced0077 Compare September 8, 2023 01:48
clojure-mode.el Outdated
(backward-sexp 1))
(setq n (1- n))))))

(defun clojure--is-top-level-form-p (&optional point)
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference with clojure-top-level-form-p? It's a bit confusing to have 2 very similarly named functions IMO.

Copy link
Contributor Author

@p4v4n p4v4n Sep 8, 2023

Choose a reason for hiding this comment

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

clojure--is-top-level-form-p checks if the sexp following the point is a top level form.
clojure-top-level-form-p checks the type of enclosing top level form w.r.t current point.
ex:
|(ns foo) -> (clojure--is-top-level-form-p) returns true.
(|ns foo) -> (clojure--is-top-level-form-p) returns nil.
(comment |(ns foo)) -> (clojure--is-top-level-form-p) returns nil.
(comment (|ns foo)) -> (clojure-top-level-form-p "comment") returns true.

Added some unit tests now for clojure--is-top-level-form-p.

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest naming it clojure--looking-at-top-level-form

This would be consistent in name and semantics with looking-at, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the fn to clojure--looking-at-top-level-form.

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

Fantastic work, kudos!

@vemv vemv merged commit 77feec4 into clojure-emacs:master Sep 11, 2023
@p4v4n p4v4n deleted the fix-clojure-find-ns-part2 branch September 11, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants