Fork me on GitHub
#code-reviews
<
2015-06-12
>
ul06:06:09

@kamillelonek: May be https://www.jetbrains.com/upsource/ will fit your needs? I have no experience with that, but JetBrains usually make good tools.

kamillelonek06:06:21

I'll check that

danielcompton07:06:46

@kamillelonek: I’ve had great results with Gerrit for code reviews, but it’s a little bit more heavyweight than a one-off code review. gerrithub is free for OSS

kamillelonek07:06:55

I was thinking about something small, gerrit is quite big as I've tried it

kamillelonek07:06:00

still, it need to be self hosted 😉

zoldar07:06:11

@kamillelonek: it is, it's downloadable

kamillelonek07:06:10

seriously, I need a dead simple solution, something like GitHub gists, but with better commenting options, I'll stick to gists for now, but in case I find something I'll post it here

zoldar08:06:54

@kamillelonek: oh crap, I got it backwards, sorry

gjnoonan19:06:46

@kamillelonek: I would be careful of your naming, and try to make it a bit clearer -> declaring github-user-public-keys-url as a def but also in the parameter of a function for instance. Think about namespaces too, eveything in here is specific to github so I wouldn’t have github in all the defn’s.

gjnoonan19:06:32

@kamillelonek:

(defn github-user-public-keys-json [github-user-public-keys-url]
  (json/read-str (github-user-public-keys-response github-user-public-keys-url) :key-fn keyword))
could become something like
(defn github-user-public-keys-json [public-keys-url]
  (let [response (response-body public-keys-url) ]
    (json/read-str response :key-fn keyword)))

gjnoonan19:06:01

github-user-public-keys-response makes more sense becoming response-body, and it is then re-usable and it doesn’t matter what you give to it (within reason)

kamillelonek19:06:35

ok, your points are very valid

kamillelonek19:06:45

thank you for the input

gjnoonan19:06:33

You’re welcome simple_smile feel free to come back for further input after you have made changes

gjnoonan20:06:11

I am not sure If json should be in the function also, as it is implied that json will be returned. Which it will not.

gjnoonan20:06:06

perhaps public-keys-for-user and then pass a username in rather than the whole url.

gjnoonan22:06:35

@kamillelonek: Sorry for the late reply, better, but still watch out for your naming.

(defn- all-members-json []
  (:body (all-members-response)))
For instance, is json needed in the name? I don’t feel it adds any value. It would be better mentioning that in the docstring.. member is being mentioned a lot in the functions, which implies to me it needs to be separated.. A few more little things too, but again relating to naming.
(defn- all-members-public-keys-urls []
  (map github/member-public-keys-url (all-members-logins)))
I feel that a verb/action needs to be the function of the map. What are you intending to do with all-members-logins ? github/member-public-keys-url doesn’t tell me.

kamillelonek22:06:02

> Sorry for the late reply

kamillelonek22:06:14

I'm so grateful you are helping me at all

kamillelonek22:06:30

I don't even expect to have an immediate response

kamillelonek22:06:57

in the first case when I'm using json is to distinguish function name from the other one without json

kamillelonek22:06:26

it's not needed at all, but I don't have a better name, all-members already exists

kamillelonek22:06:57

naming is kinda hard for me, when I come from OO where it was easier to have objects and name their methods

kamillelonek22:06:20

especially naming modules and namespaces is what I'm still getting used to

gjnoonan22:06:48

Which is why I think something like a response-body function, which grabs the body of any response you pass into it may be a better option.

gjnoonan22:06:20

member-public-keys-with-id wheres the id from? does it take an id or is it returned with an id?

kamillelonek22:06:37

hm, it returns id as well

gjnoonan23:06:40

is this always going to be used just for getting public keys?