search

pluginaweek / state_machine

Adds support for creating state machines for attributes on any Ruby class
http://www.pluginaweek.org
MIT License
3.74k stars 507 forks source link

Fix validation visibility in AS >= 4.1 #313

Open blackxored opened 10 years ago

rud commented 10 years ago

There are a lot of unrelated whitespace changes in this PR, would you be able to clean that up so only the semantic change is included?

blackxored commented 10 years ago

I think the whitespace changes are welcome, as trailing whitespace is not something you might want to have there, in addition, all my editors automatically fix it on save. Can you just check git diff -b master? I also think Github has an option for that. Thanks.

On Fri, Jun 20, 2014 at 12:11 PM, Laust Rud Jacobsen < notifications@github.com> wrote:

There are a lot of unrelated whitespace changes in this PR, would you be able to clean that up so only the semantic change is included?

— Reply to this email directly or view it on GitHub https://github.com/pluginaweek/state_machine/pull/313#issuecomment-46663827 .

Best Regards,

Adrian Perez Twitter: @blackxored | Skype: blackxored | Phone: +1 (559) 744-3201

rud commented 10 years ago

I don't have commit-bit, just wanted to provide a spot of feedback on your commit which might make it a no-brainier to merge if a maintainer ever gets the time to review.

Den 20/06/2014 kl. 17.32 skrev Adrian Perez notifications@github.com:

I think the whitespace changes are welcome, as trailing whitespace is not something you might want to have there, in addition, all my editors automatically fix it on save. Can you just check git diff -b master? I also think Github has an option for that. Thanks.

On Fri, Jun 20, 2014 at 12:11 PM, Laust Rud Jacobsen < notifications@github.com> wrote:

There are a lot of unrelated whitespace changes in this PR, would you be able to clean that up so only the semantic change is included?

— Reply to this email directly or view it on GitHub https://github.com/pluginaweek/state_machine/pull/313#issuecomment-46663827 .

Best Regards,

Adrian Perez Twitter: @blackxored | Skype: blackxored | Phone: +1 (559) 744-3201 — Reply to this email directly or view it on GitHub.

RLovelett commented 10 years ago

@blackxored I agree that the whitespace changes are probably necessary. However, I agree with @rud the whitespace stuff is cruft when compared with the pull request that you actually submitted.

My suggestion would be to turn this into to 2 commits. One to modify the whitespace and another to make the patch in question. Going forward I would recommend the use of git add -p. It provides the capability to stage different chunks of a modified file.

clarketus commented 10 years ago

Hey guys, I recently upgraded my app to rails 4 and I am having to link to this PR in the Gemfile to get the app working. It would be great to remove the git option from the dependency!

I was wondering whats blocking getting this merged? Do you need any help?

Cheers.

rud commented 10 years ago

No repository changes in the last year, think this is abandoned by the maintainer. http://rubygems.org/gems/finite_machine seems like a simpler replacement.

clarketus commented 10 years ago

Ah, I didn't notice that. Thanks for the suggestion.