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

Fix :include-macros error in cljc and cljs #1834

Merged
merged 4 commits into from Oct 7, 2022

Conversation

NoahTheDuke
Copy link
Contributor

@NoahTheDuke NoahTheDuke commented Oct 6, 2022

Please answer the following questions and leave the below in as part of your PR.


Closes #1830

Some jumbled thoughts:

Went to fix the above and realized that this should probably be expanded to a new linter so I wrote it. Makes me wonder if we're gonna need either a "all :require lints" linter with various options or if this step-by-step process is fine.

There's a couple levels of granularity to the config, which feels messy. :include-macros, :allow-clojure, :unknown-require-options, and which dialect is being checked all affect this, meaning we have 16 different combinations (some of which have the same outcome). Is there a way to lower this?

I noticed that for :refer-macros, we're not throwing any warnings if in a .clj file, so that's an area for improvement later down the line. Maybe this shouldn't be :include-macros specific but :dialect-require-options or something?

Excised the new linter, just throwing :syntax error if :include-macros's opt isn't literal true. Also checks for cljc and cljs now.

I changed the :unknown-require-option default to :off as all new linters should be off by default.

@NoahTheDuke NoahTheDuke changed the title Implement :include-macros linter Fix :include-macros error in cljc and cljs Oct 7, 2022
@borkdude
Copy link
Member

borkdude commented Oct 7, 2022

You probably need to correct the capital letter in the tests

@NoahTheDuke
Copy link
Contributor Author

Should be fixed now.

@borkdude
Copy link
Member

borkdude commented Oct 7, 2022

@borkdude borkdude merged commit 4de142d into clj-kondo:master Oct 7, 2022
@NoahTheDuke NoahTheDuke deleted the nb/fix-include-macros-warn branch October 7, 2022 15:52
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.

Ignore :include-macros option in the unknown require options linter
2 participants