cpjk / canary

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

Add preload option #17

Closed Fabi755 closed 9 years ago

Fabi755 commented 9 years ago

Add option for preloading given associations. That's helpfull for loading resources with related data and having more informations in the can? function.

cpjk commented 9 years ago

I like this idea, but there are a few things that I feel need to be done:

1) The preload option should be optional; it is currently required, and things break if it is not provided in opts. This could be done with a function like:

defp preload_if_needed(records, repo, opts)
  # only preload if :preload key is found in opts
  # otherwise return unchanged records
end

which would be used instead of repo.preload/2. Or something along those lines.

2) This should be fully tested to ensure that it works as advertised (please see the current tests for examples).

Fabi755 commented 9 years ago

Thank you for feedback.

Yes the repo.get_env(...) was wrong.

I had the changes only tested on my project, there works. But you're right, the tests are important.

1) The :preload option is already optional you can set nil without any errors. I move the preload using to new function, that's a cleaner solution.

2) I have added some tests, I hope there are useful enough :-)

cpjk commented 9 years ago

Just a few small comments, and then I think it is looking good! :smile:

Fabi755 commented 9 years ago

Thanks for feedback and support!

I changed the marked lines. Sorry for my bad english ;-)

cpjk commented 9 years ago

LGTM :+1:

Thanks for the PR!