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

Add FFI::Platform.windows? (fixes #2064) #2066

Merged
merged 4 commits into from Dec 2, 2012

Conversation

r3n4ud
Copy link
Contributor

@r3n4ud r3n4ud commented Dec 1, 2012

No description provided.

@brixen
Copy link
Member

brixen commented Dec 1, 2012

The spec/core/ffi files are from before the spec/ruby/optional/ffi specs. The spec/core/ffi specs are deprecated and will be removed. Could you ensure that the specs are in spec/ruby/optional/ffi or add them?

@r3n4ud
Copy link
Contributor Author

r3n4ud commented Dec 1, 2012

Thank you @brixen

I've just added FFI::Platform specs. (git question: is this right to rebase before adding some commits for you?)

@brixen
Copy link
Member

brixen commented Dec 1, 2012

@nibua-r yes, please rebase to master HEAD.

@r3n4ud
Copy link
Contributor Author

r3n4ud commented Dec 1, 2012

@brixen Isn't it what I have done?

@brixen
Copy link
Member

brixen commented Dec 1, 2012

@nibua-r I'm not sure, looks weird here. I'll merge manually.

@r3n4ud
Copy link
Contributor Author

r3n4ud commented Dec 1, 2012

Thanks @brixen That's strange, I'll check that back at home.

@r3n4ud
Copy link
Contributor Author

r3n4ud commented Dec 1, 2012

Strange. Anyway, thank you. I'll check if some action is required when back aT home.

Brian Ford notifications@github.com a écrit :

@nibua-r I'm not sure, looks weird here. I'll merge manually.


Reply to this email directly or view it on GitHub:
#2066 (comment)

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

@dbussink
Copy link
Contributor

dbussink commented Dec 2, 2012

One more thing, apparently you changed / touched lib/ext/melbourne/grammar18.cpp, that's probably not intended.

@r3n4ud
Copy link
Contributor Author

r3n4ud commented Dec 2, 2012

I'm really sorry I'll fix this ASAP

Dirkjan Bussink notifications@github.com a écrit :

One more thing, apparently you changed / touched
lib/ext/melbourne/grammar18.cpp, that's probably not intended.


Reply to this email directly or view it on GitHub:
#2066 (comment)

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

@r3n4ud
Copy link
Contributor Author

r3n4ud commented Dec 2, 2012

Should be fixed now.


platform_is :darwin do
it "returns true" do
Copy link

Choose a reason for hiding this comment

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

Looks like you've made a typo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please be more precise? For me, I'm testing a module method so I use FFI::Platform.windows?

Copy link

Choose a reason for hiding this comment

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

The platform is darwin. Why would #windows? return true? (the test itself actually asserts that it should be false).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arff, my bad! I'll fix this now… anyway, who cares about darwin ;) No kidding: thank you and I'm sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The typo was there before me: https://github.com/rubinius/rubinius/blob/master/spec/core/ffi/platform_spec.rb#L36-L41
… but I didn't catch it…

I'll review and fix this.

@brixen brixen merged commit 7fd1ba9 into rubinius:master Dec 2, 2012
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.

None yet

3 participants