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

Infer :style/indent metadata #787

Merged
merged 5 commits into from
Jul 28, 2023
Merged

Infer :style/indent metadata #787

merged 5 commits into from
Jul 28, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Jul 27, 2023

Implements #777

Most of the work was shipped in clojure-emacs/orchard#169

@vemv vemv requested a review from bbatsov July 27, 2023 06:08
@vemv
Copy link
Member Author

vemv commented Jul 27, 2023

Moved to draft, it looks good but something is off from what I'm observing in local usage.

@vemv vemv marked this pull request as ready for review July 27, 2023 17:50
@vemv
Copy link
Member Author

vemv commented Jul 27, 2023

Ready again!

:else
(some-> m :name namespace symbol))
str)]
(not (or (string/starts-with? namespace-name "clojure.")
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need special handling for the built-in macros?


(def clojure-core (try (find-ns 'clojure.core)
(catch Exception _e nil)))

;;; Auxiliary

(defn- var-meta-with-fn
"Like clojure.core/meta but adds {:fn true} for functions and macros.
(defn- inferrable-indent? [m]
Copy link
Member

Choose a reason for hiding this comment

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

If we end up keeping this function (I'm not sure special handling for built-in namespaces is a good idea), let's add a docstring for it.

@vemv
Copy link
Member Author

vemv commented Jul 28, 2023

Thanks for the feedback! Added a commit, please check it out

@bbatsov
Copy link
Member

bbatsov commented Jul 28, 2023

I get that clojure-mode has hardcoded indents for Clojure built-ins, but what about other clients? That's my main concern here. Probably that's something that should be handled in CIDER, not here.

@vemv
Copy link
Member Author

vemv commented Jul 28, 2023

I'm not sure of how would I infer the style/indent for clojure.core macros:

  • they can have complex arglists
  • I have nothing to compare them against

@vemv
Copy link
Member Author

vemv commented Jul 28, 2023

...So it seems reasonable to me to not infer anything for this subset of macros.

Which is worth emphasizing, does not change anything when compared to traditional cider-nrepl behavior (which always lacked indent inference)

@bbatsov
Copy link
Member

bbatsov commented Jul 28, 2023

But some follow the standard naming for params (e.g. body, forms)?

Which is worth emphasizing, does not change anything when compared to traditional cider-nrepl behavior (which always lacked indent inference)

I get this. I'm just trying to understand what makes those macros special. :-)

@vemv
Copy link
Member Author

vemv commented Jul 28, 2023

But some follow the standard naming for params (e.g. body, forms)?

Hmm, yes.

But I'd have to check clojure.* macros one by one, to verify that they match with clojure-mode's hardcoded list.

(any mismatch would be a pain)

...We could copy the whole clojure-mode list into orchard.indent. Although it would mean keeping the two lists in sync.

If you are on board with that, I could do it.


Although it would mean keeping the two lists in sync

(we could try parsing the .el file from Clojure for bonus DRY points)

@bbatsov
Copy link
Member

bbatsov commented Jul 28, 2023

I don't want to open extra work for you. I guess for the needs of CIDER this makes sense and we can think down the road if we want to expand the inference. I can live with the PR in its current form.

@vemv
Copy link
Member Author

vemv commented Jul 28, 2023

Cheers. It's certainly valuable to discuss these, anyway.

I'll like to learn more about other cider-nrepl consumers, next months. I have the impression that they don't reach out to us much

@bbatsov
Copy link
Member

bbatsov commented Jul 28, 2023

@PEZ (from Calva) is probably the person who communicates most with our team. Not sure if anyone monitors the tracker here, though.

@vemv vemv merged commit eb229e5 into master Jul 28, 2023
@vemv vemv deleted the infer-indent branch July 28, 2023 13:08
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