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.

Sign up for GitHub

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

Jump to bottom

Enable most clj-kondo linters that are disabled by default #201

Merged
merged 5 commits into from
Jan 19, 2022
Merged

Enable most clj-kondo linters that are disabled by default #201

merged 5 commits into from
Jan 19, 2022

Conversation

vemv
Copy link
Contributor

@vemv vemv commented Dec 25, 2021 •
edited
Loading

Brief

  • Enable most clj-kondo linters that are disabled by default
  • Upgrade various dependencies
    • It is important to keep our CI representative of how an end-user dep tree should look like.
    • Also keeps antq and nvd-clojure mostly happy.
    • I didn't update Timbre as 5.0.0 seems to involve some breaking changes.

QA plan

I ran lein install and verified that the reduce linter is reported:

Screen Shot 2021-12-25 at 12 40 58 PM

Author checklist

  • I have QAed the functionality
  • The PR has a reasonably reviewable size and a meaningful commit history
  • I have run the branch formatter and observed no new/significative warnings
  • The build passes
  • I have self-reviewed the PR prior to assignment
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

Reviewer checklist

  • I have checked out this branch and reviewed it locally, running it
  • I have QAed the functionality
  • I have reviewed the PR
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

vemv added 2 commits December 25, 2021 12:38
@vemv
Enable most clj-kondo linters that are disabled by default
51328b9 
One can learn about them in https://github.com/clj-kondo/clj-kondo/blob/v2021.12.19/doc/linters.md
@vemv
Upgrade various dependencies
af9279a 
Keeps antq and nvd-clojure mostly happy.

I didn't update Timbre as 5.0.0 seems to involve some breaking changes.
vemv
vemv commented Dec 25, 2021
View reviewed changes
Copy link
Contributor Author

@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.

Will be a draft pr until clj-kondo/clj-kondo#1514 is solved

src/formatting_stack/linters/kondo.clj Outdated
:unused-referred-var off ;; already offered by clean-ns
:unresolved-namespace off ;; already offered by clean-ns
;; https://github.com/clj-kondo/clj-kondo/blob/v2021.12.19/doc/linters.md
:linters {:docstring-no-summary warn ;; disabled by default
Copy link
Contributor Author

@vemv vemv Dec 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Probably the only controversial one would be :docstring-no-summary. I agree with the Clojure Style Guide that complete sentences lead to more comprehensible code.

Feel free to deliberate

Edit: also relevant clj-kondo/clj-kondo#1516

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't add :docstring-no-summary finally, it might be feasible if it adds some extra options

@vemv vemv marked this pull request as ready for review December 27, 2021 15:22
@vemv vemv marked this pull request as draft December 27, 2021 15:23
@vemv vemv marked this pull request as ready for review January 16, 2022 03:32
@vemv
Copy link
Contributor Author

vemv commented Jan 16, 2022

Ready!

@thumbnail thumbnail requested a review from neominik January 19, 2022 08:29
neominik
neominik approved these changes Jan 19, 2022
View reviewed changes
Copy link

@neominik neominik left a comment

Choose a reason for hiding this comment

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

Enabled linters and updated dependencies seem reasonable. Formatting still works as expected.

@thumbnail thumbnail merged commit 38cac4c into nedap:main Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neominik neominik neominik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vemv @neominik @thumbnail