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

Linting fix #23

Merged
merged 17 commits into from
May 16, 2018
Merged

Linting fix #23

merged 17 commits into from
May 16, 2018

Conversation

dotemacs
Copy link
Contributor

@dotemacs dotemacs commented May 11, 2018

Amended formatting of the code in the project in order to keep Eastwood happy.
All of the eastwood warnings have been resolved.

All of the cljfmt issues resolved aswell.

@bbatsov
Copy link
Contributor

bbatsov commented May 12, 2018

Thanks for tackling this! Let me know when the PRs ready for review!

@dotemacs
Copy link
Contributor Author

Will do.

@dotemacs
Copy link
Contributor Author

dotemacs commented May 15, 2018

NOTE: after discussing the below on Slack, I've just reused clojure.tools.logging instead of keeping this function around which sort of reinvents the logging.

There are only two eastwood warnings left:

* src/main/clojure/clojure/tools/nrepl/misc.clj:11:11: redefd-vars: Var log def'd 2 times at line:col locations: clojure/tools/nrepl/misc.clj:8:13 clojure/tools/nrepl/misc.clj:11:11
* src/main/clojure/clojure/tools/nrepl/misc.clj:8:13: bad-arglists: Function on var log defined taking # args [2 :or-more] but :arglists metadata has # args [0 :or-more]

Which is related to this function: https://github.com/clojure-emacs/nREPL/blob/380e161dae0f67c07645e2b4eb615797665ca989/src/main/clojure/clojure/tools/nrepl/misc.clj#L6-L17

From my conversation with the eastwood maintainers:

So is there a way to say: in this namespace, disable warnings for :redefd-vars, in a configuration file ?

The only way I can think of is to do a separate Eastwood run on that namespace, disabling warnings completely for the namespace.
Then for your run on "most of your namespaces", exclude that namespace.
So 2 separate Eastwood runs.
Sorry, I know Eastwood could really use a facility for fine-grained disabling of individual warnings, like other lint tools have (e.g. via comments you add to the code with a special linter-recognized syntax), but never got around to implementing that.

Would you like to do that, have two configs:

  • one for the whole project except misc.clj
  • and another for just misc.clj ?

Or do you have some other suggestion?
Thanks

@dotemacs dotemacs changed the title Eastwood fix Linting fix May 16, 2018
Removal of whitespace and blanklines. Not intentional but my editor
picked it up when saving.

Grouping them all here under a single commit for brevity.
And to prevent it from issuing warnings
I've done this in order to suppress a warning from Eastwood
Reorganised the require so that it makes Eastwood happy
Reorganised the namespace declaration so that Eastwood would not
complain
Eastwood warning:

src/main/clojure/clojure/tools/nrepl/middleware.clj:151:29:
local-shadows-var: local: comparator invoked as function shadows var:

src/main/clojure/clojure/tools/nrepl/middleware.clj:151:29:
local-shadows-var: local: comparator invoked as function shadows var:

Renamed the argument in order to resolve the above.
Eastwood error:

  src/main/clojure/clojure/tools/nrepl/transport.clj:42:49:
  local-shadows-var: local: read invoked as function shadows
  var: #'clojure.core/read

Fix for the above.
Added # in front of doall.

Added additional ';' in front of a comment so that it's indented
nicely.

And then the rest are just indentation issues.

Eastwood warning:

src/test/clojure/clojure/tools/nrepl/load_file_test.clj:12:3:
unused-ret-vals: Should use return value of function call, but it is
discarded: (doall (nrepl/message timeout-session {:op "load-file",
:file "\n\n\n(defn function [])"}))

src/test/clojure/clojure/tools/nrepl/load_file_test.clj:24:3:
unused-ret-vals: Should use return value of function call, but it is
discarded: (doall (nrepl/message timeout-session {:op "load-file",
:file "\n\n\n\n\n\n\n\n\n(defn afunction [])", :file-path
"path/from/source/root.clj", :file-name "root.clj"}))

src/test/clojure/clojure/tools/nrepl/load_file_test.clj:36:3:
unused-ret-vals: Should use return value of function call, but it is
discarded: (doall (nrepl/message timeout-session {:op "load-file",
:file (slurp (File. project-base-dir
"load-file-test/clojure/tools/nrepl/load_file_sample.clj")),
:file-path "clojure/tools/nrepl/load_file_sample.clj", :file-name
"load_file_sample.clj"}))

src/test/clojure/clojure/tools/nrepl/load_file_test.clj:52:3:
unused-ret-vals: Should use return value of function call, but it is
discarded: (doall (nrepl/message session {:op "load-file", :file "(def
a (+ 1 (+ 2 (+ 3 (+ 4 (+ 5 6))))))\n (def b 2) (def c 3) (def
^{:internal true} d 4)", :file-path "path/from/source/root.clj",
:file-name "root.clj"}))
Eastwood error:

src/main/clojure/clojure/tools/nrepl/middleware/interruptible_eval.clj:145:9:
unused-ret-vals-in-try: Should use return value of static method call,
but it is discarded inside body of try: (. java.lang.Class (forName
"java.util.ServiceLoader"))
Switched to assert here as per Eastwood's documentation
Not only was it not being used, it had the :reason part outside the
disable-warning config section
But adding it to the ignore config of eastwood so that if it needs to
be removed it'll be easier.
There are two while functions within clojure.tools.nrepl-test which
macroexpand to an if clause which is a constant logical true.

Adding this warning suppression in order to keep eastwood happy.
The main reason for implementing this is because eastwood was warning
on the two issues:

* src/main/clojure/clojure/tools/nrepl/misc.clj:11:11: redefd-vars:
  Var log def'd 2 times at line:col locations:
  clojure/tools/nrepl/misc.clj:8:13 clojure/tools/nrepl/misc.clj:11:11

* src/main/clojure/clojure/tools/nrepl/misc.clj:8:13: bad-arglists:
  Function on var log defined taking # args [2 :or-more] but :arglists
  metadata has # args [0 :or-more]

And I don't see the reason to reinvent logging, so just going with
clojure.tools.logging.
This way it's more consistent with piggieback project so when it comes
to removing/fix it, it can be done on both at the same time.
Ran lein cljfmt fix as that is one of the steps in the CI run.

It was mostly just indentation issues. Since it's across the whole
codebase, doing it in one commit.
cljfmt changed the order of the namespace arguments.

Cleaned it up so that the name of the ns goes first followed by
attr-map and then by require
@bbatsov bbatsov merged commit c812c3f into nrepl:master May 16, 2018
@dotemacs dotemacs deleted the eastwood-fix branch May 16, 2018 08:19
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.

2 participants