Support for configurable column names #73
Conversation
| @@ -25,9 +25,29 @@ module Transitions | |||
| extend ActiveSupport::Concern | |||
|
|
|||
| included do | |||
| class ::Transitions::Machine | |||
| unless instance_methods.include?(:new_initialize) | |||
There was a problem hiding this comment.
3 questions regarding this:
1.) In case new_initialize is predefined, doesn't this mean the "name your state column how you want functionality" will fail badly at runtime without giving the user a chance to find out what he can actually do about it?
2.) If you test for preventing an accidental overriding new_initialize, shouldn't you do the same as well for new_update?
3.) Are you really sure this test makes sense at all? I can see what you're doing I am just not so sure about it...
There was a problem hiding this comment.
1.) In case new_initialize is predefined, doesn't this mean the "name your state column how you want functionality" will fail badly at runtime without giving the user a chance to find out what he can actually do about it?
My point here was that I'd consider it an application bug in transitions itself if we re-opened Transitions::Machine in multiple places and hence I am not so sure that we should check this at all.
There was a problem hiding this comment.
I just found that in the tests the code that aliases these functions would be executed more than once, resulting in an endless loop.
I also considered just adding it to the code to Transitions::Machine in general but outside of active record it's not really necessary since every state machine has its own name and tracks its own state.
There was a problem hiding this comment.
I just found that in the tests the code that aliases these functions would be executed more than once, resulting in an endless loop.
Alright - I assume you'll update you're this PR?
:)
I also considered just adding it to the code to Transitions::Machine in general but outside of active record it's not really necessary since every state machine has its own name and tracks its own state.
Yes, exactly.
Also, you haven't responded to my question from above:
Do we have to check this? Should we?
Right now I'd say no, but perhaps I'm missing something...
There was a problem hiding this comment.
Like I said, the test is there to make sure the aliasing only happens once.
This seems to happen with the tests reliably for some reason and there is nothing keeping a user from requiring the file twice and then ending up with an endless loop.
Alright - I assume you'll update you're this PR?
This one I didn't get :)
Back to your original questions one and two:
- I think if somebody wants to modify Transitions::Machine, they should understand the code first, before doing so. I will add a warning about this in the logs, though, to give people a hint.
- Checking both might make more sense if we want to cover all cases. Will add that too.
There was a problem hiding this comment.
Great, let me know when that's finished and I'll merge..;)
There was a problem hiding this comment.
It should be there already. Could you check if you see the commit with 8cdcd1a ?
|
This looks excellent! |
|
Oh, and one more thing: Could you also please add a short section in the README? |
|
Excellent! Going to rebase and squash in a second. |
|
@cmw the problem is that you used a rather old version of transitions - could you please rebase your feature branch against current master? |
|
I did? Sorry for that. I thought I had updated the source tree first. I'll do so right away |
|
Alright. So now it should be up to date. Tests still look okay, save for some notifications. |
| end | ||
|
|
||
| def set_initial_state | ||
| self.state ||= self.class.get_state_machine.initial_state.to_s if self.respond_to?(:state=) | ||
| self[transitions_state_column_name] ||= self.class.get_state_machine.initial_state.to_s if self.has_attribute?(transitions_state_column_name) |
There was a problem hiding this comment.
This should be:
.....if self.respond_to?(transitions_state_column_name)
otherwise we would have a regression, see here for details: #68
There was a problem hiding this comment.
Ah, thanks. Didn't know about that issue. Interesting behavior and good to know. So, is there anything else I can help with or are we all set with this fix?
|
Rebased, squashed and merged into master: 1304a15 |
|
awesome, you're welcome |
This should fix #54
I settled for
instead of
:columnI also set mocha to a specific version because newer ones break with the configured version of test-unit