search

ueberauth / guardian

Elixir Authentication
MIT License
3.43k stars 382 forks source link

New permission types #585

Closed Hanspagh closed 5 years ago

Hanspagh commented 5 years ago

I have started to refactor the bitwise permission into its own module. This will to allow other encodings like json, as suggest here #470

Currently it is very much work in progress, but as of now I have moved the bitwise encoding to its own module and all the old tests parses. Next task is to implement a json encoder and then make the interface simpler to make it easier for people to write their own encoders.

To avoid breaking changes I didn't change the module name from bitwise to permissions, but maybe it is something we should consider?

doomspork commented 5 years ago

@Hanspagh I will look through this later today or tomorrow but I'm loving the refactoring so far! I'm all for improving the maintainability of the code. 🎉

doomspork commented 5 years ago

@Hanspagh now would be the time to change the module name. While V2 is merged, it has not been released, and this could be included as V2 is a breaking change.

Thoughts?

cc @yordis

yordis commented 5 years ago

Definitely, prefer to do the changes that would introduce breaking change as V2

Hanspagh commented 5 years ago

I have now renamed the base permission module and added text and atom encoding modules. Fell free to have a look.

Hanspagh commented 5 years ago

Any feedback on this?

yordis commented 5 years ago

@Hanspagh sorry, I wasn't aware if this was ready for code review or not. I will do some CR today.

doomspork commented 5 years ago

Sorry @Hanspagh, like @yordis I saw the WIP I thought this was still being worked on. I removed the label and I'll look this over today.

yordis commented 5 years ago

@doomspork should we use do ... end block over do: all the time?

This is the only thing that I would give feedbacks so far but I am not sure if that should be the case.

doomspork commented 5 years ago

@yordis only if it is multiple lines. I think trying to use , do: with long single-line functions makes it difficult to read.

Hanspagh commented 5 years ago

I sorry for the long delay, I will fix the last requested changes today

Hanspagh commented 5 years ago

Fixed the pipelines issues, bad habit of just putting everything in a pipe

yordis commented 5 years ago

@Hanspagh travis is failing btw

codecov-io commented 5 years ago

Codecov Report

Merging #585 into master will increase coverage by 0.7%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #585     +/-   ##
=========================================
+ Coverage   84.97%   85.68%   +0.7%     
=========================================
  Files          17       20      +3     
  Lines         406      426     +20     
=========================================
+ Hits          345      365     +20     
  Misses         61       61
Impacted Files Coverage Δ
lib/guardian/permissions/permissions.ex 90.47% <ø> (ø)
lib/guardian/permissions/bitwise_encoding.ex 100% <100%> (ø)
lib/guardian/permissions/text_encoding.ex 100% <100%> (ø)
lib/guardian/permissions/atom_encoding.ex 100% <100%> (ø)

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 82c1f5c...52ede25. Read the comment docs.

Hanspagh commented 5 years ago

Travis should be happy now

doomspork commented 5 years ago

Thank you @Hanspagh!!

doomspork commented 5 years ago

@yordis would you like to prep the next release?

yordis commented 5 years ago

@doomspork Aye!

Then I need to close https://github.com/ueberauth/guardian_phoenix/issues/2