Append the path to the github url with slash when necessary #10
Conversation
phmeier-nubank
force-pushed
the
phmeier/join-path-with-slash
branch
2 times, most recently
from
7883ac3
to
96eba1b
Compare
phmeier-nubank
force-pushed
the
phmeier/join-path-with-slash
branch
from
96eba1b
to
b214302
Compare
phmeier-nubank
force-pushed
the
phmeier/join-path-with-slash
branch
from
b214302
to
9e280a3
Compare
This is to avoid having to pass |
| (if (success-codes (:status response)) | ||
| (update response :body (partial parse-body (content-type response))) | ||
| (throw (ex-info "Request to GitHub failed" | ||
| {:response (select-keys response [:status :body])} | ||
| (:error response)))))) | ||
|
|
||
| (defn new-client [{:keys [app-id private-key token org] :as opts}] | ||
| (defn new-client [{:keys [app-id private-key token org token-fn] :as opts}] | ||
| {:pre [(or token app-id token-fn)]} |
There was a problem hiding this comment.
instead of just app-id shouldn't be (and app-id private-key org)?
There was a problem hiding this comment.
Good catch, I think it's actually (or token token-fn (and app-id private-key)).
Do you have a use case for this already? |
| @@ -11,14 +11,20 @@ | |||
| (defn get-installation-token [{:keys [token-fn]}] | |||
| (token-fn)) | |||
|
|
|||
| (defn- append-url-path [baseurl path] | |||
There was a problem hiding this comment.
Wdyt about adding some unit tests for these cases
(append-url-path "github.com" "path")
(append-url-path "github.com/" "path")
(append-url-path "github.com" "/path")
There was a problem hiding this comment.
@gabrielgiussi this is a private function and implicitly tested in https://github.com/nubank/clj-github/pull/10/files#diff-3d82479c9bb9ffd50aac8b0bc3dacf8754ae47fb4c95de6794a0f7d25e870a4cR27. Also the github base url is defined as a constant, I don't think adding these tests add much value.
|
I'm ok with allowing overwriting the headers but would like to better understand what's the use case for allowing the path without the initial slash. |
| @@ -11,14 +11,20 @@ | |||
| (defn get-installation-token [{:keys [token-fn]}] | |||
| (token-fn)) | |||
|
|
|||
| (defn- append-url-path [baseurl path] | |||
| (defn- append-url-path [baseurl path] | ||
| (str baseurl (when-not (or (.endsWith baseurl "/") | ||
| (.startsWith path "/")) | ||
| "/") path)) |
There was a problem hiding this comment.
To help make it distinguishable that path has nothing to do with the when-not expression could you either have one arg per line for the str function, or let-bind the when-not expr?
I'm going to update the PR desc, this was part of the PR initially until I removed it. I had trouble with the github authorization before and specifying the Authorization header manually was what did it for me. Now that this code is no longer included the PR desc actually does not make sense. |
I'm going to remove the tests as well. |
/where necessary