Skip to content
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

New permission types #585

Merged
merged 11 commits into from
May 1, 2019
Merged

Conversation

Hanspagh
Copy link
Contributor

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
Copy link
Member

@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. 🎉

@yordis yordis mentioned this pull request Mar 12, 2019
@doomspork
Copy link
Member

@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
Copy link
Member

yordis commented Mar 12, 2019

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

@Hanspagh
Copy link
Contributor Author

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

@Hanspagh
Copy link
Contributor Author

Any feedback on this?

@yordis
Copy link
Member

yordis commented Mar 25, 2019

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

@doomspork doomspork changed the title [WIP] New permission types New permission types Mar 25, 2019
@doomspork
Copy link
Member

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.

Copy link
Member

@doomspork doomspork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hanspagh this is awesome seriously. I absolutely would love to see more refactors like this. The resulting code is so easy to read and walk through, the tests are focused and concise.

Two small syntax changes and this is 👍 from me.

lib/guardian/permissions/text_encoding.ex Outdated Show resolved Hide resolved
lib/guardian/permissions/text_encoding.ex Outdated Show resolved Hide resolved
@yordis
Copy link
Member

yordis commented Mar 27, 2019

@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
Copy link
Member

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

@Hanspagh
Copy link
Contributor Author

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

@Hanspagh Hanspagh force-pushed the other-permission-types branch from cf51ecc to e4781cd Compare April 12, 2019 11:13
@Hanspagh
Copy link
Contributor Author

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

yordis
yordis previously approved these changes Apr 12, 2019
@yordis yordis requested a review from doomspork April 12, 2019 16:58
@yordis
Copy link
Member

yordis commented Apr 12, 2019

@Hanspagh travis is failing btw

@Hanspagh Hanspagh force-pushed the other-permission-types branch from 5c28421 to 8768b4c Compare April 15, 2019 08:15
@Hanspagh Hanspagh force-pushed the other-permission-types branch from 40ee0b6 to 52ede25 Compare April 15, 2019 09:12
@codecov-io
Copy link

codecov-io commented Apr 15, 2019

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
Copy link
Contributor Author

Travis should be happy now

@doomspork
Copy link
Member

Thank you @Hanspagh!!

@doomspork doomspork merged commit b7a6128 into ueberauth:master May 1, 2019
@doomspork
Copy link
Member

@yordis would you like to prep the next release?

@yordis
Copy link
Member

yordis commented May 1, 2019

@doomspork Aye!

Then I need to close ueberauth/guardian_phoenix#2

Hanspagh added a commit to Hanspagh/guardian that referenced this pull request Aug 30, 2019
* update ex_doc to be able to generate docs

* refactor bitwise_endcoding into own module

* move validation back

* move most logic back into bitwise, and keep decoding and encoding logic in relevant modules

* dynamicly load modules

* rename permission module and add text and atom encoders

* dont use pipelines for singel functions

* add simple module docs

* fix behavoir types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants