Fork me on GitHub
#code-reviews
<
2016-12-08
>
roelofw16:12:16

here a url where you can see the site in action : https://lit-lowlands-46138.herokuapp.com/

gdeer8118:12:16

Sorry @roelofw I didn't realize I wasn't in this channel

gdeer8118:12:02

I like to pull down repos and run them locally but it looks like I can't since I need an api key. First thing I suggest is to update the readme with the purpose, if you can run it locally, design decisions you made, etc.

roelofw18:12:40

I can give you the same api key as I use in a pm

snoe18:12:54

@roelofw you can pass a :query-params map instead of building the query-string manually e..g here https://github.com/rwobben/paintings/blob/master/src/clj/paintings2/api_get.clj#L39

roelofw18:12:36

@snoe : oke , can I then have the same url as before

roelofw18:12:10

you mean the key and format part ?

roelofw18:12:50

oke, I will hit the manual of clj-http how to do that

snoe18:12:08

you can drop everything after the '?' and put it in a map like {:format "json" :key apikey}

roelofw18:12:51

oke, but the key must be a secret so I have to pull it out with environ

roelofw18:12:34

so it will be (client/get {:as :json :format "json" :key (env :key}) as I understand your right ?

roelofw18:12:40

and the sentece above it , will be (str " id "/tiles?")

snoe18:12:31

not quite (client/get (str "" id "/tiles") {:as :json :query-params {:format "json" :key (env :key)}}

roelofw18:12:11

oke, I can do the same for the other urls , right ?

snoe18:12:16

right, yeah

roelofw18:12:25

thanks, I will try it

snoe18:12:22

the rest is pretty good for a first project. I'd look into your editor and find a way to auto-format removing extra spaces and trailing parens

roelofw18:12:05

changed it and will try to build it

roelofw18:12:59

and it worked

roelofw18:12:14

now I will change the rest and update my repo

roelofw18:12:44

@snoe : thanks for the remarks , so I learn more and more about clojure

snoe18:12:17

@roelofw you might also want to look at http-kit over clj-http http://stackoverflow.com/a/21473485 depends how many ids you're doing in parallel.

roelofw18:12:53

moment, one problem at the time

snoe18:12:29

I'm not suggesting you do, just know it's available

roelofw18:12:32

@snoe : can you see if I made a error here :

( str "")
                 (client/get {:as :json :query-params {:key (env :key) :format "json" :type "schilderijen" :toppieces "True"}}) 

snoe18:12:40

drop the ?

roelofw18:12:42

Ì see now a empty output

roelofw18:12:39

oke, I changed it and try again

roelofw18:12:51

hmm, still empty output

snoe18:12:36

type=schilderij vs {:type "schilderijen"} ?

roelofw18:12:12

@snoe : you mean like this : (client/get {:as :json :query-params {:key (env :key) :format "json" "type=schilderijen" :toppieces "True"}})

roelofw18:12:58

nope, I see then a error message

snoe18:12:03

no, in github you didn't have the en at the end of the value

snoe18:12:10

you were using schilderij / but you just pasted schilderijen

roelofw18:12:43

oke, I did things too fast

roelofw18:12:48

I will change it

roelofw19:12:23

thanks, that worked well

roelofw19:12:10

@snoe , Do you want to see the new code ?

snoe19:12:44

sounds like you got it, but I did notice this last pmap should probably be a map https://github.com/rwobben/paintings/blob/master/src/clj/paintings2/api_get.clj#L54 because merge is so fast

roelofw19:12:09

oke, I can change that also

roelofw19:12:58

and the maxium api calls that run in parallel are 10

roelofw19:12:24

IM looking at http-kit and try to figure out how to change the code t

snoe19:12:48

if the max is 10, i'd stick with clj-http

roelofw19:12:58

Thanks for all the remarks

snoe19:12:05

no problem

roelofw19:12:04

and here the changed code with your name mentioned : https://github.com/rwobben/paintings

roelofw19:12:12

you can be right, I takes a map and returns a map

roelofw19:12:24

I have to rewrite it then

roelofw19:12:20

the only thing I have to figure out is how to deal with 2 variables result and id.

roelofw19:12:42

as far as I know map can only take a anymous function with 1 parameter

roelofw19:12:42

What happens is that I have a id of this type : NL-SK-C-5 and I need this SK-C-5

roelofw19:12:19

so I can do (map (fn [item] (subs id 3) )

snoe19:12:56

results is just the intermediate vector you're building up inside the reduce right?

snoe19:12:20

I think you're close with that map

roelofw19:12:36

yes, as I see it result is the accumelator

roelofw19:12:56

I think also, Im only afraid I loose the rest of the map

snoe19:12:59

how's that? map-id is the list of the NL- prefixed ids and you want to subs each one.

snoe19:12:26

you can do almost anything to a list with reduce, map filter etc are just a level of abstraction above. especially when learning clojure if you ever feel the need to use reduce first look through clojure.core for a function that fits what you want to do more closely

roelofw20:12:02

No the map looks like this { id: "NL-SK-C-5" : painter "Rembrandt" :year 1658 }

roelofw20:12:18

and I only have to change the id and keep the rest

roelofw20:12:28

or do we misunderstood each other

snoe20:12:31

I'm pretty sure read-numbers is returning a vector of ids like ["SK-C-5"]

roelofw20:12:33

you are right, Im confusing functions . Too late here to think well

roelofw20:12:01

@snoe : I think this one is better (map (fn [item] (subs (:id item) 3) )

roelofw20:12:07

I will try it

roelofw20:12:00

hmm, that gives a empty seq , even as (map (fn [item] (subs item) 3 ) [] map-id)]

gdeer8120:12:40

in read-numbers, how are you able to use the :artObjects key when the body is a json string?

gdeer8120:12:55

I also noticed in the json response that there is a key called "objectNumber" that is exactly what you're trying to get when you substring the id

roelofw20:12:29

Because client/get { :as json}) convert the json to a clojure object

gdeer8120:12:55

that's odd, it's not doing that at the repl

gdeer8120:12:36

might be because I'm bypassing the middle layer wrappers at the repl

roelofw20:12:27

can be map-id looks like this:

("nl-SK-A-2963"
 "nl-SK-C-2"
 "nl-SK-C-149"
 "nl-SK-A-3841"
 "nl-SK-C-1368"
 "nl-SK-A-1718"
 "nl-SK-A-1115"
 "nl-SK-A-799"
 "nl-SK-A-3584"
 "nl-SK-A-2860") 

roelofw20:12:12

oke, this one works (map (fn [item] (subs item 3 )) map-id)

roelofw20:12:07

@snoe : does this look well or can I improve it

roelofw20:12:53

@gdeer81 what output do you get from read-numbers then ?

gdeer8120:12:36

at the repl you get json. I was pulling out the object numbers with (re-seq #"SK-[A-Z]-[0-9]{0, 4}" body-of-response)

roelofw20:12:46

json respons from this

( str "")
                 (client/get {:as :json :query-params {:key (env :key) :format "json" :type "schilderij" :toppieces "True"}})  

roelofw20:12:55

that is very wierd

gdeer8121:12:14

also, I'm running a bare repl with only clj-http as a dep, I'm not running it from the project

gdeer8121:12:50

side note, why are you calling str when you only have one string?

roelofw21:12:27

o, that is a left over from another remark

roelofw21:12:15

I think I can delete that

gdeer8121:12:57

okay, I see where you're concating the id. wow the codebase keeps mutating out from under me lol

roelofw21:12:30

yes, that is for me the learning process

roelofw21:12:47

learn how I can do things "better"

roelofw21:12:49

@gdeer81 this better :

(defn read-numbers
  "Reads the ids of the paintings"
  []
  (let [data (-> (client/get  "" {:as :json :query-params {:key (env :key) :format "json" :type "schilderij" :toppieces "True"}})
                 :body
                 :artObjects
                 )
        map-id (map :id data)
        ids (map (fn [item] (subs item 3 )) map-id) ]
    ids )) 

hiredman21:12:04

:as :json is dependent on the cheshire library being present

gdeer8121:12:34

@hiredman thank you. Okay I spun up my repl with lein try "clj-http" "cheshire" and it gave me enough dependencies to test the function

gdeer8121:12:18

@roelofw Spoiler alert, you can do all of that with (->> (client/get "" {:as :json :query-params {:key (env :key) :format "json" :type "schilderij" :toppieces "True"}}) :body :artObjects (map :objectNumber))

hiredman21:12:11

you can lose the partial and use ->>

gdeer8121:12:37

okay, updated the snippet

roelofw21:12:02

@gdeer81 oke, and object-number is the number without the NL part ?

roelofw21:12:56

and can I make test for testing all functions

roelofw21:12:14

I did that is not going to work because of the external api

roelofw22:12:01

@gdeer81 yes, this is also working :

(defn read-numbers
  "Reads the ids of the paintings"
  []
  (->> (client/get  "" {:as :json :query-params {:key (env :key) :format "json" :type "schilderij" :toppieces "True"}})
                 :body
                 :artObjects
                 (map :objectNumber)))  

roelofw22:12:17

the code gets shorter and shorter

roelofw22:12:24

Time to sleep now

roelofw22:12:31

Thanks all

gdeer8122:12:02

until next time

roelofw22:12:37

if you have other remarks , write them down here or make a issue

roelofw22:12:57

Have a good day, @gdeer81