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 salt as an array of bytes instead of String #18

Merged
merged 4 commits into from Apr 6, 2016

Conversation

andredsp
Copy link
Contributor

@andredsp andredsp commented Feb 9, 2016

Use salt as an array of bytes instead of String. Also added arrayFromString to ease use of existing code samples.

Note: it's my first pull request. Sorry if I'm messing something.

@iosdevzone
Copy link
Owner

Hi there,

First, congrats on you first pull request!

Your request points out a missing API in the current library (thank you!), one that accepts the salt as an array of bytes, but your solution, rather than adding this capability, replaces an existing one. For that reason, I am not going to merge the request "as is".

Instead I will add the new API alongside the existing one, or if you would like to modify your pull request so that it does this, I will merge it.

I hope this is clear, let me know if you have any questions.

Cheers,
idz

@andredsp
Copy link
Contributor Author

andredsp commented Feb 9, 2016

I see your point. If you give me some time, I make the changes myself and request a new pull.

@iosdevzone
Copy link
Owner

Great. Look forward to the new pull request. I'll leave this open for the moment as a reminder of the issue and close this and the new request together.

Recover previous derivedKey method with String salt for backwards
compatibility
@andredsp
Copy link
Contributor Author

I pushed the previous code to my branch, and it showed in this pull request. Do I have to create a new one?

@iosdevzone
Copy link
Owner

No, you don't have to create a new one. Just one more thing and this can be merged. You need to test both methods. Your previous commit removed tests for the derivedKey method that took the salt as a string. Please combine the previous tests with your new ones to make sure everything is tested.

Thanks for your help on this. I think this is a useful addition to the library!

@andredsp
Copy link
Contributor Author

You're right. I'll add the tests as soon as I get some time, hopefully in a couple of days.

@iosdevzone
Copy link
Owner

Hi andredsp,
Just checking in. I would like to close this pull request. I can complete the edit but, since you mentioned this was your first pull request I wanted to give you ample opportunity to do it yourself.
I understand if you've just gotten too busy or something other project is taking your time. Just let me know.
Cheers,
Danny

@AccentureAPinto
Copy link
Contributor

Hi Danny,
I was really busy last couple of months. Now I'm on vacation and found some time to make a new pull, but my arrayFromString code is crashing on Swift 2.2 (Xcode 7.3). I fixed it but want to test in previous Xcode+Swift. With luck I finish it today, or on my next free-time window in a few days.

@andredsp
Copy link
Contributor Author

andredsp commented Apr 4, 2016

Do you need any other action on my side?

@iosdevzone
Copy link
Owner

Hi Andre,
Sorry, been busy myself the last few days. I think that looks good. I'll just do a few more checks and merge the pull request!
Danny

@iosdevzone iosdevzone merged commit cf09559 into iosdevzone:master Apr 6, 2016
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