ueberauth / guardian

Elixir Authentication
MIT License
3.43k stars 382 forks source link

support 1.4 #533

Closed ghost closed 5 years ago

ghost commented 5 years ago

529

codecov-io commented 5 years ago

Codecov Report

Merging #533 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #533   +/-   ##
=======================================
  Coverage   85.19%   85.19%           
=======================================
  Files          18       18           
  Lines         412      412           
=======================================
  Hits          351      351           
  Misses         61       61

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 73b0c3d...e0223d8. Read the comment docs.

yordis commented 5 years ago

Hey @paulZzhang thanks a lot for the PR but this is the rc version, not an official version of the package.

ghost commented 5 years ago

@yordis Please publish a rc version.Thanks.

yordis commented 5 years ago

@paulZzhang test is failing.

Could you change to be just >= 1.3.0?

ghost commented 5 years ago

@yordis the branch c692ff4 is >=1.3

taiansu commented 5 years ago

@yordis the branch c692ff4 is >=1.3

According to my understanding and experiments, >= 1.3 is an invalid syntax. We have to specify the patch version number, e.g. >= 1.3.0 while using the >= operator.

In short, {:phoenix, "~> 1.0 or >= 1.2.0", optional: true}, works on my machine (elixir 1.7.4, phoenix 1.4.0, erlang 21.1.1).

michalmuskala commented 5 years ago

According to the documentation of the Version module, the requirement ~> 1.0 is exactly the same as >= 2.0.0 and < 3.0.0. So ~> 1.0 or >= 1.2.0 is effectively exactly the same as ~> 1.0 alone.

yordis commented 5 years ago

@paulZzhang sorry maybe I wasn't clear about it, I meant to change to just >=1.3.

 {:phoenix, ">= 1.3.0", optional: true},

But, @ueberauth/core are talking about this and we will move this dependency outside of guardian into guardian_phoenix package.

doomspork commented 5 years ago

@yordis are you satisfied with this change now? If so let's merge

yordis commented 5 years ago

@doomspork that is Phoenix 1.3 that does not include 1.4 ...

jrissler commented 5 years ago

It would be nice to have this merged - I'm in the middle of upgrading to 1.4 now and this is a blocker :)

taiansu commented 5 years ago

@yordis Actually this change does include phoenix 1.3 and 1.4. According to version module that @michalmuskala mentioned, the current change ~> 1.3 means all version >= 1.3.0 and < 2.0.0.

Despite the fact that previous change ~> 1.0 is the one which makes better backward compatibility, my two cents on this is fix this in whatever way you like asap, instead of being a phoenix upgrade roadblock for no observable feature impact, 3+ characters. 😄

jrissler commented 5 years ago

Thanks!