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

Use AASM::NO_VALUE as to_state argument default value #484

Merged
merged 3 commits into from
Jul 27, 2017

Conversation

teohm
Copy link
Contributor

@teohm teohm commented Jul 26, 2017

Based on discussion in #449 (comment)

This PR attempts to fix the root cause of a few issues (#449, #416, #441) with this similar error:

Wrong number of arguments (given 1, expected 0)

This error happens because AASM incorrectly calls guard method with
one extra argument in certain scenarios. For example:

def can_do_something?  # guard method with no argument
  ...
end

# But AASM incorrectly triggers `can_do_something?(nil)`

This happens because in AASM::Core::Event#_fire(obj, options={}, to_state=nil, *args),
the to_state argument default nil value is causing ambiguity, i.e. there is no way for
AASM to tell whether a caller provides a) one argument with nil value, or b) no argument.

This PR removes this ambiguity by using a symbol as to_state default value.

@@ -39,18 +39,18 @@
it 'should be able to wait into waiting if polite' do
auth.left_suspend!
expect(auth.may_left_wait?(:waiting, :please)).to be true
auth.left_wait!(nil, :please)
Copy link
Contributor Author

@teohm teohm Jul 26, 2017

Choose a reason for hiding this comment

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

@anilmaurya I noticed nil is explicitly included in argument list when triggering event in the specs. Is this a common in real usage?

This is different from the usage examples written in https://github.com/aasm/aasm/wiki/State-transitions-with-guard-clauses-that-require-arguments

Copy link
Member

Choose a reason for hiding this comment

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

@teohm Including nil in argument list seems weird to me.

@anilmaurya anilmaurya merged commit ed88361 into aasm:master Jul 27, 2017
@anilmaurya
Copy link
Member

Thank you @teohm for contributing , I appreciate your help 🎖

@teohm
Copy link
Contributor Author

teohm commented Aug 3, 2017

@anilmaurya By any chance to release this fix some time this month August? 🙏

@anilmaurya
Copy link
Member

Hi @teohm ,

I am planning to release new version with this fix soon.

@teohm
Copy link
Contributor Author

teohm commented Aug 3, 2017

That's great! thanks @anilmaurya !! 🎉

@anilmaurya
Copy link
Member

Hi @teohm , just release 4.12.2 with this fix.

@teohm
Copy link
Contributor Author

teohm commented Aug 4, 2017

Thanks @anilmaurya !!

@teohm teohm deleted the aasm_no_value branch August 4, 2017 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants