search

cpjk / canary

:hatching_chick: Elixir authorization and resource-loading library for Plug applications.
MIT License
473 stars 52 forks source link

Add configurable current user #14

Closed mgwidmann closed 9 years ago

mgwidmann commented 9 years ago

Fixes #12

mgwidmann commented 9 years ago

I put them into their own module as to keep the import Mock isolated to the few tests that need it.

I can rebase it to add mock into a separate commit but the tests will fail without Mock. Theres no way to run async: true and not mock out the global app config, which is why I included it.

Should I also group the other tests into nested modules based on what they are testing? Its my understanding this is ExUnit's way of describe...

cpjk commented 9 years ago

Regarding the nested module, I like the idea for isolating imports etc..

After some more thought, I think it is fine to add the nested test module without putting the other tests in their own nested modules.

Regarding the separate commit for adding the mock dependency, if you put the "Add mock to project" commit before the commit where mock is actually used, then there shouldn't be any problems. Maybe I'm missing something?

So, the only remaining comment is RE: adding mock in its own commit :smiley:

mgwidmann commented 9 years ago

Reorganized the commits like you asked. I changed one thing though. Instead of letting it do can?(nil, action, resource) we raise an exception to let the developer know they may have configured something incorrectly. If you prefer we can make this a logger warning.

mgwidmann commented 9 years ago

Nevermind, I changed it over to use Dict.fetch!/2 so that it acts like it did previously...

I figured out that can?(nil, action, resource) means can anyone access this action, resource... All better now!

cpjk commented 9 years ago

Ok, looking good!

However, I would rather not merge commits that introduce breaking changes, specifically, the "raise ..." change, which you fixed with 3a2d35091d2ebfdde8759571a483133ef0713595. This is to avoid confusion later on in the case of a revert.

Could you please squash 697640279e1863263d897b83cd9f22b99b740ef3 and 3a2d35091d2ebfdde8759571a483133ef0713595 so that the "raise ..." change is not in the history?

One last small thing: convention is to keep commit messages in the imperative tense, e.g. "Add thing" instead of "Added thing", and short (52 characters) on the first line. This keeps the commits messages consistent with those generated by git commands like merge.

mgwidmann commented 9 years ago

Hope everything is well now... Let me know if there is anything else

cpjk commented 9 years ago

Looks good to me. Thanks for the PR!