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

Reduce Clojure linter warning noise #17491

Merged
merged 4 commits into from
Oct 4, 2023
Merged

Conversation

ilmotta
Copy link
Contributor

@ilmotta ilmotta commented Oct 3, 2023

Summary

This PR changes the make lint target default behavior to NOT show clj-kondo warnings.

In the CI I kept the same behavior, i.e. show all warnings and errors simultaneously.

Motivation

When devs run make lint, most of the time, they don't want to see a long list of warnings. Their focus is on the errors. Additionally, the majority of devs in the mobile team see clj-kondo warnings in their editors of choice already.

We (some of us) believe the editor feedback/warnings are sufficiently noisy.

But I don't like this new default!

Okay, add the following somewhere in your config files:

export CLJ_LINTER_PRINT_WARNINGS=true

Do you think we should invert the logic?

I can also change this PR to invert the logic, i.e. we keep the current behavior (a bit noisy) and then devs can export a CLJ_LINTER_HIDE_WARNINGS=true in their own environments if they want less noise.

Areas that may be impacted

None

Steps to test

  • make lint
  • make lint CLJ_LINTER_PRINT_WARNINGS=true

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Oct 3, 2023

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 8788a4e #1 2023-10-03 21:08:50 ~6 min android-e2e 🤖apk 📲
✔️ 8788a4e #1 2023-10-03 21:08:58 ~6 min android 🤖apk 📲
✔️ 8788a4e #1 2023-10-03 21:11:14 ~8 min ios 📱ipa 📲
✔️ 8788a4e #1 2023-10-03 21:13:32 ~11 min tests 📄log
✔️ 30ab933 #2 2023-10-04 11:22:43 ~6 min android 🤖apk 📲
✔️ 30ab933 #2 2023-10-04 11:22:51 ~6 min android-e2e 🤖apk 📲
✔️ 30ab933 #2 2023-10-04 11:27:15 ~10 min ios 📱ipa 📲
✔️ 30ab933 #2 2023-10-04 11:29:34 ~13 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 2f2f4dc #3 2023-10-04 15:13:26 ~5 min android 🤖apk 📲
✔️ 2f2f4dc #3 2023-10-04 15:13:29 ~5 min android-e2e 🤖apk 📲
✔️ 2f2f4dc #3 2023-10-04 15:14:45 ~7 min ios 📱ipa 📲
✔️ 2f2f4dc #3 2023-10-04 15:17:28 ~9 min tests 📄log
✔️ e09032e #4 2023-10-04 18:55:31 ~5 min android-e2e 🤖apk 📲
✔️ e09032e #4 2023-10-04 18:55:57 ~5 min android 🤖apk 📲
✔️ e09032e #4 2023-10-04 18:56:59 ~6 min ios 📱ipa 📲
✔️ e09032e #4 2023-10-04 18:59:24 ~9 min tests 📄log

Copy link
Contributor

@mohsen-ghafouri mohsen-ghafouri left a comment

Choose a reason for hiding this comment

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

Excellent :)) ❤️

Copy link
Contributor

@yqrashawn yqrashawn left a comment

Choose a reason for hiding this comment

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

LGTM

although I tried run clj-kondo --config .clj-kondo/config.edn --cache false --fail-level error --lint src
it seems the clj-kondo in status-mobile nix shell doesn't show warnings
my own one (newer version) do show warnings

Copy link
Member

@flexsurfer flexsurfer left a comment

Choose a reason for hiding this comment

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

image

@ilmotta
Copy link
Contributor Author

ilmotta commented Oct 4, 2023

LGTM

although I tried run clj-kondo --config .clj-kondo/config.edn --cache false --fail-level error --lint src it seems the clj-kondo in status-mobile nix shell doesn't show warnings my own one (newer version) do show warnings

Thanks for reporting that @yqrashawn. I was super puzzled by that too, but then I found a fix in the Makefile. If we add a missing ampersand between the clj-kondo and the previous command , the target prints the warnings (see commit 2f29d3e).

@ilmotta ilmotta force-pushed the chore/reduce-warning-noise branch from 8788a4e to 30ab933 Compare October 4, 2023 11:16
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@ilmotta ilmotta force-pushed the chore/reduce-warning-noise branch from 30ab933 to 2f2f4dc Compare October 4, 2023 15:07
@ilmotta ilmotta force-pushed the chore/reduce-warning-noise branch from 2f2f4dc to e09032e Compare October 4, 2023 18:49
@ilmotta ilmotta merged commit aa2345d into develop Oct 4, 2023
2 checks passed
@ilmotta ilmotta deleted the chore/reduce-warning-noise branch October 4, 2023 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants