update mix deps & refactor code #264
Conversation
doomspork
left a comment
doomspork
left a comment
There was a problem hiding this comment.
Thank you @alexandrubagu, I have some feedback for you. Let me know if you have any questions.
mix.exs
Outdated
| [{:jose, "~> 1.8"}, | ||
| {:phoenix, "~> 1.2.0", optional: true}, | ||
| {:plug, "~> 1.0"}, | ||
| [{:jose, "~> 1.8.1"}, |
There was a problem hiding this comment.
@alexandrubagu do we need to include the patch version? 1.8 should cover 1.8.x
lib/guardian/claims.ex
Outdated
| defp assign_exp_from_ttl(the_claims, _), do: the_claims | ||
|
|
||
| end | ||
|
|
There was a problem hiding this comment.
Please remove extra whitespace.
lib/guardian/claims.ex
Outdated
|
|
||
| @doc false | ||
| def ttl(%{"iat" => iat_v} = the_claims, requested_ttl) do | ||
| def ttl(the_claims = %{"iat" => iat_v}, requested_ttl) do |
lib/guardian.ex
Outdated
| {headers, claims} = strip_value(claims, "headers", %{}) | ||
| {secret, claims} = strip_value(claims, "secret") | ||
| {_, token} = secret | ||
| #tuple {header, claims} |
There was a problem hiding this comment.
What is the reasoning behind this change? Seems unnecessary.
lib/guardian.ex
Outdated
| stringify_params = stringify_keys(params) | ||
|
|
||
| #tuple {secret, params} | ||
| secret_params = strip_value(stringify_params, "secret") |
There was a problem hiding this comment.
Why did you remove the pattern matching?
There was a problem hiding this comment.
Let's go back to using pattern matching here, it's not clear to me why it was removed in the first place. In fact the resulting code is worse without it, we're now reliant on using pipe chains within function calls to get the values we want.
|
After I've updated credo, I run So, I try to fix them. If you want to update deps to have the latest ones, I'm glad to make a new commit with required changes Please let me know what are your intentions |
doomspork
left a comment
doomspork
left a comment
There was a problem hiding this comment.
@alexandrubagu still a few more things we need to change before this PR is in a merge-able state.
lib/guardian.ex
Outdated
| stringify_params = stringify_keys(params) | ||
|
|
||
| #tuple {secret, params} | ||
| secret_params = strip_value(stringify_params, "secret") |
There was a problem hiding this comment.
Let's go back to using pattern matching here, it's not clear to me why it was removed in the first place. In fact the resulting code is worse without it, we're now reliant on using pipe chains within function calls to get the values we want.
lib/guardian.ex
Outdated
| try do | ||
| with {:ok, claims} <- decode_token(jwt, secret), | ||
| {:ok, verified_claims} <- verify_claims(claims, params), | ||
| with {:ok, claims} <- decode_token(jwt, secret_params |> elem(0)), |
There was a problem hiding this comment.
We do not want to use pipes inside function calls, this is messy.
|
I've revert changes and set some credo checking rules to false |
|
Thank you @alexandrubagu! I'll look over this later this evening and we get it merged if we're all good to go 👍 @hassox feel free to take a peek at this. |
|
@doomspork Any updates on this? What about upgrading to |
|
@snewcomer I still need to look through this PR but we are not making changes for 1.3rc, please see the previous threads on the topic: #276 |
mix.exs
Outdated
| [{:jose, "~> 1.8"}, | ||
| {:phoenix, "~> 1.2.0", optional: true}, | ||
| {:plug, "~> 1.0"}, | ||
| {:phoenix, "~> 1.2.1", optional: true}, |
There was a problem hiding this comment.
@alexandrubagu could we update this to: {:phoenix, "~> 1.2", optional: true}? I believe it'll address most of the 1.3rc1 concerns at the same time 👍
|
Thank you @alexandrubagu! |
|
Thank you again @alexandrubagu and apologizes for the delays. |
|
No problem |
* update mix deps * add new credo config * refactor code to pass new credo specs * add jose 1.8.1 which fix warnings on elixir 1.4 * revert to parttern matching * set some checks to false because needs refactor * update to phoenix 1.2
4 participants
update mix deps to new ones and refactor a bunch of code