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

[WIP]: Make HelloService a bit more varied/conversational #17

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

Conversation

aranasaurus
Copy link
Contributor

No description provided.

@IanKeen
Copy link
Contributor

IanKeen commented Oct 3, 2016

Fail is due to arc4random not being available on linux, the following should work:

#if os(Linux)
import Glibc
#else
import Darwin.C
#endif

fileprivate extension Array {
    var randomElement: Element {
        #if os(Linux)
        let index = Int(random() % self.count)
        #else
        let index = Int(arc4random_uniform(UInt32(self.count)))
        #endif
        return self[index]
    }
}

but check out Int.random(min:max) first and see if that might also work?

@aranasaurus
Copy link
Contributor Author

Yeah, that was one of the things that I knew I was going to have to address. Hadn't looked for the cross-platform Swift way yet.

@aranasaurus
Copy link
Contributor Author

My plan was to move that extension to Common, after I make it crossplatform

@IanKeen
Copy link
Contributor

IanKeen commented Oct 3, 2016

awesome! 👍 😎

@aranasaurus
Copy link
Contributor Author

aranasaurus commented Oct 4, 2016

So, Int.random(min:max:) works, but that comes from Vapor. So I'd need to add Vapor as a dependency to Common, in order to move it there. Which would you rather: do the #if os dance or add the dependency, or don't move it up to the framework and leave it at the Camille level?

I'm leaning toward the dance, seems like a giant hammer for a thumbtack.

@IanKeen
Copy link
Contributor

IanKeen commented Oct 4, 2016

ahh you're right.. I removed my Int.random due to a conflict - add the randomElement extension - if it builds on linux I'll move it to Common

@aranasaurus
Copy link
Contributor Author

I was already mid-PR creation so... there. :)

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.

3 participants