-
Notifications
You must be signed in to change notification settings - Fork 26
Cleanup: Fix core-test Deps Order, Remove Unused, etc.
#798
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
base: main
Are you sure you want to change the base?
Conversation
| #?@(:cljs [] | ||
| :clj [179769313486231570000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000N r/max-double] | ||
| :clj [179769313486231570000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000N r/max-double]) | ||
| :cljr [179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368N r/max-double] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added cljr specific largest double
| (ns clojure.core-test.bound-fn | ||
| (:require [clojure.test :as t] | ||
| [clojure.core-test.portability #?(:cljs :refer-macros :default :refer) [when-var-exists]])) | ||
| (:require [clojure.test :as t :refer [deftest is testing]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converted to refer for consistency with the rest of the project
| (t/is (= @fut :unset) "bound-fn stays bound even in other thread")))) | ||
| (testing "Threaded/future cases" | ||
| (let [f (bound-fn [] *x*) | ||
| fut (future (f))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed redundant let
| (t/is (= @fut :unset) "bound-fn stays bound even in other thread")))) | ||
| (testing "Threaded/future cases" | ||
| (let [f (bound-fn* test-fn) | ||
| fut (future (f))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed redundant let
core-test Deps Order, Remove Unused, etc.
| (is (ratio? (- 0 1/3))) | ||
| (is (ratio? (- 0N 1/3))) | ||
| (is (ratio? (- 1 1/3))) | ||
| (is (ratio? (- 1N 1/3))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason not to use this over the more verbose instance? ...
| false true | ||
| true false | ||
| #?(:clj (Object.) | ||
| :cljr (Object.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed like an oversight so added it
| :clj Thread/sleep | ||
| :cljr Thread/sleep) | ||
| n)) | ||
| :cljs #(js/setTimeout identity %) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:cljr was repeated twice for some reason and there was no cljs option so added something
|
Thanks, Emma. :) I worked through all of the emails for PRs and discussions today, which ultimately led to a conflict for this PR. I will review the PR now, but we'll need to fix that conflict before we can merge. |
jeaye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Emma!
| @@ -1,8 +1,9 @@ | |||
| #_{:clj-kondo/ignore [:unused-referred-var]} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't something we should have to put in every file which isn't using a referred var. If we want kondo to be clean, we should disable these diagnostics at a project level. Same goes for things like inline defs and unused values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we catch genuine issues then (since there are cases where we'd want this to warn)? Or would it be better just to live with those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the odd nature of this code base, where we're specifically writing incorrect code, I'd rather we just live with them.
This is a large PR full of a ton of small refactorings, mostly for consistency, some just general cleanup.
Given how large this already is, I'll handle
string-testin a separate PRMost changes fit into one of the following, and if they don't I've left a comment to make this easier to review:
:referlisting to be alphabetical:defaultor be treated the same between platforms (most often this is collapsing:cljand:cljrinto:default)clj-kondoignore comments so the files are easier to read for folks usingclj-kondoand so that actual warnings appear differently and don't get lostclojure.core/xinstead ofx)xto_x