CMR-9776: Resolve linter errors & warnings in common-app-lib; add linting config #2102

zimzoom · 2024-04-24T15:17:50Z

Overview

What is the feature/fix?

Fix clj-kondo errors and warnings on common-app-lib.

Also added .clj-kondo to codebase, including config file and imported config from clj-rewrite.

What is the Solution?

See above, followed clj-kondo docs. See comments in config.edn file.

What areas of the application does this impact?

Quality control

Checklist

[x ] I have updated/added unit and integration tests that prove my fix is effective or that my feature works
[x ] New and existing unit and int tests pass locally and remotely with my changes
[x ] I have removed unnecessary/dead code and imports in files I have changed
[x ] I have performed a self-review of my own code
[x ] I have commented my code, particularly in hard-to-understand areas
[x ] I have made corresponding changes to the documentation
[x ] My changes generate no new warnings

eereiter · 2024-04-24T15:36:07Z

.clj-kondo/config.edn

+           cmr.common.jobs/def-stateful-job clojure.core/defn
+           cmr.common.util/are3 clj-kondo.lint-as/def-catch-all}
+ ;; dynamically generated functions/vars
+ :linters {:unresolved-var {:exclude [digest/md5 digest/sha-1]}}}


This is much better than the declare statements that I was using.
Can we also add are2 and are? They are deprecated functions, but the test code still uses them and are showing up as such from the linters.

Couple of questions

When you merge this in does this work when running kondo only from the top level of the project or can kondo be run inside of each project and it will pick up this file? Or do you have to tell it about the file? I am using my own installed cli-kondo executable at the moment.

After you merge in and I understand the answer to my first question I am assuming that I should remove the declare statements that I put in?

Interestingly, I would have thought it would only work from the root level as well, but I just tried it and it does still work in the subprojects. I am using my own clj-kondo executable as well and have not added clj-kondo as a dep to CMR, just this config file since clj-kondo is now our officially supported linter to my knowledge.

I tested this by changing into the common-app-lib directory and running clj-kondo --lint ./src, which still suppressed all the stuff mentioned here. To make sure I tried deleting the contents of config.edn and running it again and it did come back.

I think that a lot of those, that you previously added, might be covered by what is here, but possibly not all. Maybe we should coordinate on rules for what to consistently ignore/exclude/include etc.

And yes, I will add are2 and are3 as well.

it is just are2. are3 you already included and are is a clojure command.

Note for Reader: discussed with reviewer here and we decided I will review the aforementioned declares that were already merged in for access-control-app, acl-lib, and es-spatial-plugin, and incorporate into this PR.

reader has noted discussion.

eereiter · 2024-04-24T15:37:07Z

.clj-kondo/rewrite-clj/rewrite-clj/config.edn

@@ -0,0 +1,5 @@
+{:lint-as


What does this file do?

This file was pulled in automatically by following the instructions in clj-kondo's project set up guide, which is here: https://github.com/clj-kondo/clj-kondo?tab=readme-ov-file#project-setup

To my understanding, this file (and the folder it is in) was written by the authors of this package clj-rewrite, which we have as a dependency, and they "exported" it so that projects using their package can import it and thus lint clj-rewrite correctly.

This seems to be the only one that exported a clj-kondo config. The super common ones like compojure are included in clj-kondo by default. The docs recommended adding this to version control. (see link above)

Originally I wanted to incorporate this into the build, but when I saw only 1 dep came out of it, I figured it wasn't worth it.

common-app-lib/src/cmr/common_app/api/request_context_user_augmenter.clj

common-app-lib/src/cmr/common_app/api/routes.clj

common-app-lib/src/cmr/common_app/app_events.clj

common-app-lib/src/cmr/common_app/humanizer.clj

common-app-lib/src/cmr/common_app/services/kms_fetcher.clj

common-app-lib/src/cmr/common_app/services/kms_lookup.clj

eereiter · 2024-04-24T16:02:19Z

common-app-lib/src/cmr/common_app/services/search.clj

  (if-let [errors (seq (qv/validate query))]
    (errors/throw-service-errors :bad-request errors)
    query))

 (defmulti search-results->response
  "Converts query search results into a string response."
-  (fn [context query results]
+  (fn [_context query _results]


I have been converting the defmulti/defmethod functions to a function with a case statement when possible (it is almost always possible since most of the time all of the defmethod functions are in the same namespace), but I will let you decide if you want to do that.

In this particular case, there do exist defmethods in other namespaces. Results handler namespaces to be exact, which account for a lot of the scattered multimethods.

eereiter · 2024-04-24T16:02:32Z

common-app-lib/src/cmr/common_app/services/search.clj

    [(:concept-type query) (qm/base-result-format query)]))

 (defmulti single-result->response
  "Returns a string representation of a single concept in the format
  specified in the query."
-  (fn [context query results]
+  (fn [_context query _results]


I have been converting the defmulti/defmethod functions to a function with a case statement when possible (it is almost always possible since most of the time all of the defmethod functions are in the same namespace), but I will let you decide if you want to do that.

In this particular case, there do exist defmethods in other namespaces. Results handler namespaces to be exact, which account for a lot of the scattered multimethods.

eereiter

There are some cases where you can purge a little more code than just use _ in front of a parameter.

codecov-commenter · 2024-04-24T17:49:13Z

Codecov Report

Attention: Patch coverage is 57.14286% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 57.80%. Comparing base (289c472) to head (3621d47).

Files	Patch %	Lines
common-app-lib/src/cmr/common_app/api/routes.clj	0.00%	8 Missing ⚠️
...-control-app/src/cmr/access_control/api/routes.clj	0.00%	3 Missing ⚠️
.../common_app/api/request_context_user_augmenter.clj	0.00%	3 Missing ⚠️
acl-lib/src/cmr/acl/acl_fetcher.clj	50.00%	1 Missing ⚠️
common-app-lib/src/cmr/common_app/api/health.clj	0.00%	1 Missing ⚠️
.../collections_for_gran_acls_by_concept_id_cache.clj	0.00%	1 Missing ⚠️
.../src/cmr/common_app/data/humanizer_alias_cache.clj	0.00%	1 Missing ⚠️
common-app-lib/src/cmr/common_app/humanizer.clj	91.66%	0 Missing and 1 partial ⚠️
...c/cmr/elastic_utils/search/es_index_name_cache.clj	0.00%	1 Missing ⚠️
...-lib/src/cmr/umm_spec/umm_to_xml_mappings/dif9.clj	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2102      +/-   ##
==========================================
- Coverage   57.81%   57.80%   -0.01%     
==========================================
  Files        1044     1044              
  Lines       70318    70311       -7     
  Branches     1966     1964       -2     
==========================================
- Hits        40653    40645       -8     
  Misses      27808    27808              
- Partials     1857     1858       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

es-spatial-plugin/project.clj

ingest-app/src/cmr/ingest/runner.clj

umm-spec-lib/src/cmr/umm_spec/umm_to_xml_mappings/dif9/data_contact.clj

umm-spec-lib/src/cmr/umm_spec/umm_to_xml_mappings/iso19115_2/additional_attribute.clj

umm-spec-lib/src/cmr/umm_spec/umm_to_xml_mappings/iso19115_2/metadata_association.clj

umm-spec-lib/src/cmr/umm_spec/url.clj

…ting config

zimzoom requested review from jmaeng72, jceaser, daniel-zamora, eereiter and TylerHeald1 April 24, 2024 15:17