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

[analysis] Improve keyword reg support for re-frame #1266

Merged
merged 5 commits into from Apr 26, 2021

Conversation

ericdallo
Copy link
Member

Fixes #1159

  • Rename :def -> :reg
  • Add support for re-frame.core functions that register a keyword as definition.

@ericdallo
Copy link
Member Author

@borkdude not sure we need to add the :ref as this seems enough to make clojure-lsp works, even so we could add in a next PR maybe?

@ericdallo ericdallo changed the title Improve keyword reg [analysis] Improve keyword reg Apr 25, 2021
@@ -19,9 +19,9 @@

(defn reg-keyword!
([k]
(reg-keyword! k true))
(reg-keyword! k nil))
Copy link
Member Author

@ericdallo ericdallo Apr 25, 2021

Choose a reason for hiding this comment

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

it didn't seem right to register the keyword as a :reg true if user just reg-keyword! :kw without the intention of register that keyword as a definition, for those cases the 2 arity seems the correct approach.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, so this arity can be removed? Would it be useful for LSP to also have ref-keyword so you can find if re-frame.core/subscribe or re-frame.core/dispatch (specifically one of those) uses the keyword? Or is this covered by find-references well enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not necessary, the find references seems enough

Copy link
Member

Choose a reason for hiding this comment

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

ok, let's remove it then

Copy link
Member Author

@ericdallo ericdallo Apr 26, 2021

Choose a reason for hiding this comment

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

@borkdude not sure we should remove the 1 arity, imagine user wants to reg a keyword but not necessarily it's a keyword that is a definition, so just (reg-keyword! :my-key) looks valid, otherwise user would always register the keyword with the :def

Copy link
Member

Choose a reason for hiding this comment

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

Why would a user ever want to "register" a keyword without any context? And didn't you rename that key to :reg?

Copy link
Member Author

Choose a reason for hiding this comment

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

ops, yeah, :reg 😅.
You are right, if user don't register it, clj-kondo will reg as a normal keyword anyway.
I'll remove it so

@ericdallo ericdallo changed the title [analysis] Improve keyword reg [analysis] Improve keyword reg support for re-frame Apr 25, 2021
@ericdallo
Copy link
Member Author

Tested with clojure-lsp and with this we have find-definition for re-frame keywords :)

@ericdallo ericdallo requested a review from borkdude April 26, 2021 12:55
@borkdude
Copy link
Member

@ericdallo Looks awesome! Could you add one more test for the :lint-as case? In case people want to use this mechanism for re-frame-like macros/fns

@ericdallo
Copy link
Member Author

Done @borkdude!

@borkdude borkdude merged commit f770773 into clj-kondo:master Apr 26, 2021
@ericdallo ericdallo deleted the improve-keyword-reg branch April 26, 2021 14:38
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.

[Analysis] Support custom :def on keywords
2 participants