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

Score cli #1

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Score cli #1

wants to merge 12 commits into from

Conversation

lordaniel
Copy link
Contributor

Finish working-skeleton for Score bot supporting parameters (org-name and token)

Copy link

@sarcilav sarcilav left a comment

Choose a reason for hiding this comment

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

This a heavy refactor before merging, please check the changes

repo (get-in event ["repo" "name"])
action (get-in event ["payload" "action"])
date (get event "created_at")]
(if (= type "PullRequestEvent")
Copy link

Choose a reason for hiding this comment

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

@lordaniel if will return a bloq with nil and it is ugly, use when when you only care for the first part of the body

action (get-in event ["payload" "action"])
date (get event "created_at")]
(if (= type "PullRequestEvent")
(if (.startsWith date "2016-10")
Copy link

Choose a reason for hiding this comment

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

Same here, and actually you can use only one conditional something like (and (= type ...) (.start....))

date (get event "created_at")]
(if (= type "PullRequestEvent")
(if (.startsWith date "2016-10")
(hash-map :actor actor :repo repo :date date)))))
Copy link

Choose a reason for hiding this comment

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

you can replace (hash-map :actor actor ...) with {:actor actor ...}

(hash-map :actor actor :repo repo :date date)))))

(defn clean-arr [arr]
(loop [init arr
Copy link

Choose a reason for hiding this comment

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

you can use a filter instead


(defn count-pr [arr-pr]
(if (> (count arr-pr) 0)
{:user (get (first arr-pr) :actor) :score (count arr-pr) :lastpr (get (first arr-pr) :date)}))
Copy link

Choose a reason for hiding this comment

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

a very neat trick when using keyword (those that start with :) in clojure is that they can be used as a functions to get the val so (get (first arr-pr) :actor) should be (:actor (first arr-r))

(println "User - Score")
(apply println (map (fn [m] (str "\n" (get m :user) " - " (get m :score)))
(sort-by :score > (filter (fn [x] (> (count x) 0))
(map count-pr (map clean-arr (map (fn [arr] (map get-pullrequest arr))
Copy link

Choose a reason for hiding this comment

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

mehhh code smell 👃


(defn count-user-pr [arr]
(loop [[h & t] arr prs '()]
(if (empty? h)
Copy link

Choose a reason for hiding this comment

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

still missing the if vs when changes

(if (> (count prs) 0)
{:user (:actor (first prs)) :score (count prs) :last (:date (last prs))})
(recur t (if (and (= (get h "type") "PullRequestEvent") (.startsWith (get h "created_at") "2016-10"))
(conj prs {:actor (get-in h ["actor" "login"]) :repo (get-in h ["repo" "name"]) :date (get h "created_at")})
Copy link

@sarcilav sarcilav Nov 8, 2016

Choose a reason for hiding this comment

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

maybe some destructuring within h definition will help, something like [[{type "type date "created_at" {name "name"} "repo" {login "login"} "actor"} & t] arr prs '()]

Copy link

Choose a reason for hiding this comment

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

an initial observation here, is that when you build something with loop to fill a collection with a tail recursion like this, it is 'manual' reduce, this should be refactored into a reduce

{:user (get (first arr-pr) :actor) :score (count arr-pr) :lastpr (get (first arr-pr) :date)}))

(defn count-user-pr [arr]
(loop [[h & t] arr prs '()]

Choose a reason for hiding this comment

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

What are h, t and arr?
What about more descriptive names?

Choose a reason for hiding this comment

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

@caipivara I guess that:
h -> head.
t -> Tail.
arr -> Array

@sarcilav
Copy link

Still waiting on the reduce refactor

@lordaniel
Copy link
Contributor Author

@sarcilav @earayo @orendon What do you think about this version, can we merge it ?

Copy link

@orendon orendon left a comment

Choose a reason for hiding this comment

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

😉 Left some comments for your consideration

"Count User Pull Request Events"
(reduce (fn [final-prs pr]
(let [{type "type" date "created_at" {name "name"} "repo" {login "login"} "actor"} pr]
(if (and (= type "PullRequestEvent") (.startsWith date "2016-10"))
Copy link

Choose a reason for hiding this comment

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

October is hardcoded, what if you make the date an argument?

(defn count-user-pr [user-events]
"Count User Pull Request Events"
(reduce (fn [final-prs pr]
(let [{type "type" date "created_at" {name "name"} "repo" {login "login"} "actor"} pr]
Copy link

Choose a reason for hiding this comment

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

this let could be avoided if you do some destructuring at function signature

{:user login :score 1 :last date}
(assoc final-prs :score (inc (:score final-prs))))
final-prs)))
'()
Copy link

Choose a reason for hiding this comment

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

what if you apply a PullRequestEvent+Date filter first?
That way the reduce will get only PullRequests within the date range, making the internal function simpler

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.

6 participants