fix enum to lower case

[chinsyo]

[jedisct1]

But... why?

It breaks all the tests and even the examples from the documentation.

[chinsyo]

But... why?

It breaks all the tests and even the examples from the documentation.

emmm... Cuz swift standard library do.

[jedisct1]

But the current names are consistent with the underlying C library.

Following Swift's convention is good, but this change is going to break all existing apps without any functional improvement.

I'll let @blochberger judge if it's worth it. But if such a change is made, it has to be made everywhere, including tests, examples and documentation.

[chinsyo]

But the current names are consistent with the underlying C library.

Following Swift's convention is good, but this change is going to break all existing apps without any functional improvement.

I'll let @blochberger judge if it's worth it. But if such a change is made, it has to be made everywhere, including tests, examples and documentation.

Thanks for your reply.

[blochberger]

But the current names are consistent with the underlying C library.

This is an implementation detail and in my case only relevant if references from the Sodium documentation would cause confusion – which I do not think is the case.

Following Swift's convention is good, but this change is going to break all existing apps without any functional improvement.

I think that this is a good point. However, some parts/ideas of the pull request might be worth giving it a bit more thought:

• The SUCCESS/FAILURE cases could be replaced by the Result type introduced in Swift 5 (SE-0235).
• default is a reserved keyword requiring special quoting – as your change shows. I think this is confusing and should be avoided. Maybe the Default case can be dropped in the enum altogether or renamed to defaultAlgorithm. Dropping the case requires more thought. Simply choosing the default algorithm for any unknown integer passed to the switch statement would be a bad choice. Calling the underlaying C-function in the function signature (this is what I do in Tafelsalz) might also not be the best choice.

I'll let @blochberger judge if it's worth it. But if such a change is made, it has to be made everywhere, including tests, examples and documentation.

I vote for closing the pull request for now. I could open issues for using the new Result type and for discussing how the Default algorithm could be handled more "swifty" – which I do not think is straight forward. What do you think @jedisct1?

[jedisct1]

If we are going to make breaking changes, using the Result type definitely looks like a good change to make.

Using quotes to work around existing keywords is painful. Renaming Default to defaultAlgorithm would work.

[blochberger]

Looking at ExitCode again, I see that it is only used internally, i.e., not communicated to the user. Regardless of which error occurred, nil is returned for all fallible functions. Hence, I think that using the Result type is not needed in that case.

[chinsyo]

Looking at ExitCode again, I see that it is only used internally, i.e., not communicated to the user. Regardless of which error occurred, nil is returned for all fallible functions. Hence, I think that using the Result type is not needed in that case.

Agree

[GoranLilja]

What's the status of this pull request? I got the impression that everyone is agreeing, yet the PR isn't merged. Friendly ping @blochberger who requested changes. 🙂