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

update mix deps & refactor code #264

Merged
merged 9 commits into from Mar 22, 2017

Conversation

Projects
None yet
4 participants
@alexandrubagu
Copy link
Contributor

alexandrubagu commented Feb 2, 2017

update mix deps to new ones and refactor a bunch of code

alexandrubagu added some commits Feb 2, 2017

@alexandrubagu alexandrubagu changed the title update mix deps update mix deps & refactor code Feb 3, 2017

@doomspork
Copy link
Member

doomspork left a 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"},

This comment has been minimized.

@doomspork

doomspork Feb 5, 2017

Member

@alexandrubagu do we need to include the patch version? 1.8 should cover 1.8.x

This comment has been minimized.

@alexandrubagu

alexandrubagu Feb 6, 2017

Author Contributor

That's right

@@ -150,3 +150,4 @@ defmodule Guardian.Claims do
defp assign_exp_from_ttl(the_claims, _), do: the_claims

end

This comment has been minimized.

@doomspork

doomspork Feb 5, 2017

Member

Please remove extra whitespace.

This comment has been minimized.

@alexandrubagu

alexandrubagu Feb 6, 2017

Author Contributor

I'm ok with this

@@ -98,7 +98,7 @@ defmodule Guardian.Claims do
end

@doc false
def ttl(%{"iat" => iat_v} = the_claims, requested_ttl) do
def ttl(the_claims = %{"iat" => iat_v}, requested_ttl) do

This comment has been minimized.

@doomspork

doomspork Feb 5, 2017

Member

What this change necessary?

{headers, claims} = strip_value(claims, "headers", %{})
{secret, claims} = strip_value(claims, "secret")
{_, token} = secret
#tuple {header, claims}

This comment has been minimized.

@doomspork

doomspork Feb 5, 2017

Member

What is the reasoning behind this change? Seems unnecessary.

stringify_params = stringify_keys(params)

#tuple {secret, params}
secret_params = strip_value(stringify_params, "secret")

This comment has been minimized.

@doomspork

doomspork Feb 5, 2017

Member

Why did you remove the pattern matching?

This comment has been minimized.

@doomspork

doomspork Feb 11, 2017

Member

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.

@alexandrubagu

This comment has been minimized.

Copy link
Contributor Author

alexandrubagu commented Feb 6, 2017

After I've updated credo, I run mix credo this is the output:
guardian

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
Thanks

@doomspork
Copy link
Member

doomspork left a comment

@alexandrubagu still a few more things we need to change before this PR is in a merge-able state.

stringify_params = stringify_keys(params)

#tuple {secret, params}
secret_params = strip_value(stringify_params, "secret")

This comment has been minimized.

@doomspork

doomspork Feb 11, 2017

Member

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.


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)),

This comment has been minimized.

@doomspork

doomspork Feb 11, 2017

Member

We do not want to use pipes inside function calls, this is messy.

@alexandrubagu

This comment has been minimized.

Copy link
Contributor Author

alexandrubagu commented Feb 13, 2017

I've revert changes and set some credo checking rules to false

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Feb 13, 2017

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.

@snewcomer

This comment has been minimized.

Copy link

snewcomer commented Mar 14, 2017

@doomspork Any updates on this? What about upgrading to phoenix 1.3.0-rc.0 as well?

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Mar 14, 2017

@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
@@ -43,16 +43,16 @@ defmodule Guardian.Mixfile do

defp deps do
[{:jose, "~> 1.8"},
{:phoenix, "~> 1.2.0", optional: true},
{:plug, "~> 1.0"},
{:phoenix, "~> 1.2.1", optional: true},

This comment has been minimized.

@doomspork

doomspork Mar 21, 2017

Member

@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 👍

This comment has been minimized.

@alexandrubagu

alexandrubagu Mar 22, 2017

Author Contributor

Done 👍

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Mar 22, 2017

Thank you @alexandrubagu!

@doomspork doomspork requested review from hassox and scrogson Mar 22, 2017

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Mar 22, 2017

@hassox / @scrogson any final comments before I merge?

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Mar 22, 2017

Thank you again @alexandrubagu and apologizes for the delays.

@doomspork doomspork merged commit fa041f0 into ueberauth:master Mar 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexandrubagu

This comment has been minimized.

Copy link
Contributor Author

alexandrubagu commented Mar 23, 2017

No problem

Hanspagh added a commit to Hanspagh/guardian that referenced this pull request Nov 1, 2017

update mix deps & refactor code (ueberauth#264)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment