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 NonEmptyList/NonEmptyMap for SecurityRequirements #319

Merged
merged 1 commit into from Jun 5, 2019

Conversation

Projects
None yet
2 participants
@kelnos
Copy link
Member

commented Jun 5, 2019

We give an Option[SecurityRequirements] to terms that might need to do stuff with security. The SecurityRequirements case class contains a list of requirements. If that list is empty, there's no point in having SecurityRequirements instance at all, so we guarantee that there will be at least one item in the list, and also guarantee that the item in the list is a non-empty map, since it's pointless for it to be empty.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@kelnos kelnos requested a review from blast-hardcheese Jun 5, 2019

@blast-hardcheese
Copy link
Collaborator

left a comment

Why do we need to order the keys? Does NonEmptyMap require ordering for consistency? It's not based on ArrayMap or some other ordered map?

@kelnos

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@blast-hardcheese yeah, NonEmptyMap requires SortedMap as input. Not sure why, I guess maybe to ensure that .head always returns a consistent value?

@kelnos kelnos force-pushed the kelnos:security-requirements-nel branch from 22ece85 to 484f0ec Jun 5, 2019

@kelnos kelnos merged commit b28aadf into twilio:master Jun 5, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@kelnos kelnos deleted the kelnos:security-requirements-nel branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.