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

Fix validation visibility in AS >= 4.1 #313

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blackxored
Copy link

No description provided.

@rud
Copy link

rud commented Jun 20, 2014

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
Copy link
Author

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 <
[email protected]> 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
#313 (comment)
.

Best Regards,

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

@rud
Copy link

rud commented Jun 22, 2014

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 [email protected]:

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 <
[email protected]> 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
#313 (comment)
.

Best Regards,

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

Reply to this email directly or view it on GitHub.

@RLovelett
Copy link

@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
Copy link

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
Copy link

rud commented Jul 23, 2014

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
Copy link

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

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.

4 participants