Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Proper specs, refactoring and a couple of new features #4

Merged
merged 20 commits into from Dec 6, 2012

Conversation

Projects
None yet
3 participants
Collaborator

sh-ft commented Dec 1, 2012

  • Added unit tests for Resource and ResourceImplementation.
  • Refactored the code (possibly too much, sorry).
  • Added several features taken from cancan, which include:
    • :through_association option;
    • :instance_name option;
    • Resource.skip_authorization_check method;
  • Fixed :singleton option handling behavior.
Owner

inossidabile commented Dec 1, 2012

What a devastating work! I will try to take a closer look as soon as possible.

Owner

inossidabile commented Dec 1, 2012

If only you did p/r per time... It contains too many subjects inside. Will take some time to research.

Besides everything else you've added two options: instance_name and through_association. Does CanCan have them or anything like them? Are they compatible?

@AlexanderPavlenko raise and help me. Time has come to make this gem sweet.

Collaborator

sh-ft commented Dec 2, 2012

Besides everything else you've added two options: instance_name and through_association. Does CanCan have them or anything like them? Are they compatible?

Both options are taken from CanCan (and are described at http://rdoc.info/github/ryanb/cancan/master/CanCan/ControllerAdditions/ClassMethods:load_resource). They behave the same way, although there are some slight differences with :through_association option handling:

  1. The one described at ryanb/cancan#723, that involves using both :through_association and :singleton options together, is fixed in heimdallr-resource. I've also just added a test to show that it works.
  2. And ryanb/cancan#128, that is fixed in CanCan, but not in heimdallr-resource. It doesn't actually have anything to do with :through_association rather than with :through option, but it still makes building a resource with :through_association option behave differently. This kind of behavior was there before my commits and I didn't change it, although I think we should discuss it later in a separate thread.
Collaborator

sh-ft commented Dec 3, 2012

I realize that this pile of commits is kind of hard to follow, but since it's too late to break it up, I'll try to walk you through it and show my thought process when I was working on it.

The first three commits up to 093fb3a only add tests. If you pull just these, you'd see there is a pending test example "loads through has_one association with :singleton option" that doesn't pass because of the bug that gets fixed in a12e248. Basically, the :singleton option doesn't work at all.

I probably should have made a pull request at that point, but I wasn't quite sure about how the code works and how to fix it at the time. So in the next five commits that follow (bcd8dea through ae1ed04), I refactor the code and add more tests, still not changing any external behavior of the library. (Actually that's not true, because in 4de49aa I took the liberty of removing the code that sets heimdallr_options on the controller, since I wasn't sure why it's there; but this particular commit can be easily reverted).

In c604f92 I add :through_association option because I needed it for my project.

Then in a12e248 I fix the :singleton bug and add more examples that test this option.

In the following two commits I refactor the code further and add a couple of test examples.

In 27cf423 I add :instance_name option because I needed it too for my project.

In 0bddd51 Resource.skip_authorization_check is added (it is taken from CanCan too, by the way). I use it to ease controller testing in my project.

The last five commits (b302730 through ae8a46e) are bug fixes and final refactoring.

Does this help?

Collaborator

sh-ft commented Dec 3, 2012

I also have to say that I think heimdallr is awesome and I'd like to use it in my projects now, although the fact that it lacks proper testing and documentation makes it harder to start using it.

@AlexanderPavlenko AlexanderPavlenko merged commit 29934b9 into inossidabile:master Dec 6, 2012

Owner

inossidabile commented Dec 6, 2012

@AlexanderPavlenko Did you check it?
@voidseeker would you like to join development?

Collaborator

AlexanderPavlenko commented Dec 6, 2012

@inossidabile yes, and everything looks fine. However, I reverted 4de49aa, because we rely on this method.

Collaborator

sh-ft commented Dec 6, 2012

@inossidabile, what do you mean?

Collaborator

AlexanderPavlenko commented Dec 6, 2012

@voidseeker I guess, write access to this repo

Owner

inossidabile commented Dec 6, 2012

@voidseeker I can give you commit rights to the repo
@AlexanderPavlenko great

Collaborator

sh-ft commented Dec 6, 2012

@inossidabile, sure. I might add a couple more commits soon.

Owner

inossidabile commented Dec 7, 2012

You can now commit directly to master. I've added you to the contributors list at c8e52d1 however I wasn't able to find any details. Please change line 112 to appropriate (roundlake/heimdallr-resource@c8e52d1#L0R112)

Collaborator

sh-ft commented Dec 7, 2012

Oh, thank you. I appreciate that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment