chore: add specs to the codebase #74

raszi · 2020-03-22T22:40:28Z

No description provided.

src/github_changelog/semver.clj

thenonameguy · 2020-03-23T12:50:17Z

test/github_changelog/conventional_test.clj

+
+(defn- sample-tag [pulls]
+  (-> (spec/sample ::core-spec/tag)
+      (assoc :pulls pulls)))


I would remove this threading.

It was more readable for me this way and I was playing it a lot since generator overrides aren't always working for some reason.

tests.edn

imrekoszo · 2020-03-23T14:31:09Z

deps.edn

-           metosin/jsonista                {:mvn/version "0.2.4"}
-           org.clojure/core.async          {:mvn/version "0.4.500"} ; clojure spec fixed
+           metosin/jsonista                {:mvn/version "0.2.5"}
+           org.clojure/core.async          {:mvn/version "1.0.567"} ; clojure spec fixed


is there still a need for the comment?

I am not sure, but I have not tested.

I think it's safe to remove

imrekoszo · 2020-03-23T14:34:21Z

src/github_changelog/conventional.clj

     :pull-request pull}
+    ;; TODO - when-let?
    (if-let [[_ type scope subject] (re-find angular-pattern title)]


I see no reason why this can't be a when-let.

Yes, I believe so. I put that as a TODO because the tests were failing and I could not verify the correctness of the function.

Because of an upstream bug grimradical/clj-semver#7

raszi · 2020-03-29T19:44:45Z

I caught a really nice issue with generative testing which was reported before grimradical/clj-semver#7

thenonameguy · 2020-03-29T20:17:49Z

I caught a really nice issue with generative testing which was reported before grimradical/clj-semver#7

That's really cool.

src/github_changelog/core.clj

thenonameguy · 2020-03-29T20:24:35Z

Please consider using explicitly defined function applications instead of partial:
My arguments:

my stylistical preference
static analysis can arity-check better
negligible performance increase: https://www.tiagoespinha.net/2016/12/clojure-beware-of-the-partial/
less characters to write for the common case of single non-fixed arity

imrekoszo

Was it necessary to set ^:no-gen on so many functions? If yes, was that because those had side effects? If still yes, consider suffixing their names with ! or, possibly even better, factor out the side effects.

Co-Authored-By: Szabó Krisztián <thenonameguy24@gmail.com>

raszi · 2020-03-29T20:30:59Z

Was it necessary to set ^:no-gen on so many functions? If yes, was that because those had side effects? If still yes, consider suffixing their names with ! or, possibly even better, factor out the side effects.

Yes, unfortunately those functions are depending on each other and side effecting. Although your comment is valid and I'll try to extract that part into a separate function and use arguments instead in a separate pull request.

src/github_changelog/conventional.clj

Co-Authored-By: Szabó Krisztián <thenonameguy24@gmail.com>

thenonameguy · 2020-03-29T20:41:49Z

src/github_changelog/github.clj

@@ -54,14 +66,11 @@
        endpoint  (pulls-url config)]
    (throttler/throttle-fn (partial issue-request endpoint) ratelimit :second)))

-(defn- get-pulls [config]
+(defn fetch-pulls [config]
  (let [call-api                        (call-api-fn config)
        first-request                   (make-request config)
        first-response                  (call-api first-request)
        {links :links first-body :body} first-response
        rest-requests                   (make-requests config links)
        rest-responses                  (pmap call-api rest-requests)]
    (into first-body (flatten (map :body rest-responses)))))


Suggested change

(into first-body (flatten (map :body rest-responses)))))

(into first-body (comp (map :body) cat) rest-responses)))))

Just putting this on your radar: https://clojuredocs.org/clojure.core/cat
This version is a bit more efficient not only because not allocating an intermediate map lazyseq, but by only flattening on a single level.

imrekoszo · 2020-03-29T21:39:01Z

src/github_changelog/conventional.clj

@@ -110,8 +108,7 @@
      (conj pulls pull))))

 (defn ^:no-gen parse-changes [config {:keys [pulls] :as tag}]
-  (->> (map (partial parse-pull config) pulls)
-       (remove nil?)
+  (->> (keep (partial parse-pull config) pulls)
       (reduce filter-reverted [])


If you rewrite filter-reverted to be pred-ish (sorta like (defn reverted? [pulls pull] ,,,)), you could write these 2 lines as (into [] (comp (keep (partial ,,,)) (remove reverted?)) pulls), I think.

Could also comp in the next line then

I also though that but filter-reverted needs to see the full keep produced seq. Therefore you can't use on it on a single element inside a transducer chain.

True, I didn't realise pulls is that seq.

thenonameguy

There are a few more nits I would address if this was my PR, but functionality-wise this looks good :)

raszi added 8 commits February 8, 2020 13:55

Add a test for transtive dependencies

70b75ce

Add specs to the codebase

16cb5a1

Improve coverage

1bb4dfb

Update dependencies

5948cc9

Add tests.edn

f239cd7

Reuse our function

51c535e

Fix badge icon

1dc2564

Remove unused requires

1cc5e3c

thenonameguy reviewed Mar 23, 2020

View reviewed changes

src/github_changelog/semver.clj Outdated Show resolved Hide resolved

thenonameguy reviewed Mar 23, 2020

View reviewed changes

tests.edn Outdated Show resolved Hide resolved

imrekoszo reviewed Mar 23, 2020

View reviewed changes

raszi added 5 commits March 28, 2020 13:36

Merge branch 'master' into add-specs

171548f

Use clojure spec to validate response

d6ba18c

Switch to when-let

b2e4de9

Fix language

78e0b2a

Use destructuring

3c830ca

imrekoszo previously approved these changes Mar 29, 2020

View reviewed changes

Re-enable kaocha.plugin.alpha/spec-test-check

d17a9e7

raszi dismissed imrekoszo’s stale review via d17a9e7 March 29, 2020 19:38

Disable generators for newer?

8556e21

Because of an upstream bug grimradical/clj-semver#7

thenonameguy reviewed Mar 29, 2020

View reviewed changes

src/github_changelog/core.clj Outdated Show resolved Hide resolved

imrekoszo previously approved these changes Mar 29, 2020

View reviewed changes

Use keep instead of map + remove

a1d3a47

Co-Authored-By: Szabó Krisztián <thenonameguy24@gmail.com>

raszi dismissed imrekoszo’s stale review via a1d3a47 March 29, 2020 20:28

thenonameguy reviewed Mar 29, 2020

View reviewed changes

src/github_changelog/conventional.clj Outdated Show resolved Hide resolved

raszi and others added 2 commits March 29, 2020 22:39

Remove unnecessary no-gen and fix spec

c7b7e6b

Use keep instead of map + remove

f2e2eee

Co-Authored-By: Szabó Krisztián <thenonameguy24@gmail.com>

thenonameguy reviewed Mar 29, 2020

View reviewed changes

More keep

87e488b

imrekoszo reviewed Mar 29, 2020

View reviewed changes

imrekoszo approved these changes Mar 29, 2020

View reviewed changes

thenonameguy approved these changes Mar 30, 2020

View reviewed changes

thenonameguy merged commit 29dcaab into ekataglobal:master Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add specs to the codebase #74

chore: add specs to the codebase #74

raszi commented Mar 22, 2020

thenonameguy Mar 23, 2020

raszi Mar 23, 2020

imrekoszo Mar 23, 2020

raszi Mar 29, 2020

thenonameguy Mar 29, 2020

imrekoszo Mar 23, 2020

raszi Mar 27, 2020

raszi commented Mar 29, 2020

thenonameguy commented Mar 29, 2020

thenonameguy commented Mar 29, 2020

imrekoszo left a comment

raszi commented Mar 29, 2020

thenonameguy Mar 29, 2020 •

edited

Loading

imrekoszo Mar 29, 2020

imrekoszo Mar 29, 2020

thenonameguy Mar 30, 2020

imrekoszo Mar 30, 2020

thenonameguy left a comment

	(into first-body (flatten (map :body rest-responses)))))
	(into first-body (comp (map :body) cat) rest-responses)))))

chore: add specs to the codebase #74

chore: add specs to the codebase #74

Conversation

raszi commented Mar 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raszi commented Mar 29, 2020

thenonameguy commented Mar 29, 2020

thenonameguy commented Mar 29, 2020

imrekoszo left a comment

Choose a reason for hiding this comment

raszi commented Mar 29, 2020

thenonameguy Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thenonameguy left a comment

Choose a reason for hiding this comment

thenonameguy Mar 29, 2020 •

edited

Loading