feat(credentials) #54: validate required endpoints #58
Conversation
|
Sorry, I've included a previous commit in the PR by mistake 🤦♂️ |
lilgallon
force-pushed
the
feature/54_validate_required_endpoints
branch
from
0b179cf
to
a570a09
Compare
|
No worries, I rebased your PR, it's fine now ;) |
| */ | ||
| class CredentialsClientService( | ||
| private val clientVersionsEndpointUrl: String, | ||
| private val clientPartnerRepository: PartnerRepository, | ||
| private val clientVersionsRepository: VersionsRepository, | ||
| private val clientCredentialsRoleRepository: CredentialsRoleRepository, | ||
| private val serverVersionsEndpointUrl: String, | ||
| private val transportClientBuilder: TransportClientBuilder | ||
| private val transportClientBuilder: TransportClientBuilder, | ||
| private val requiredOtherPartEndpointsProvider: suspend () -> Map<InterfaceRole, List<ModuleID>> |
There was a problem hiding this comment.
is there a particular reason why need this as a function? instead of just a map? ... I'd assume this will be static anyways, as your particular implementation will require certain endpoints, or not
nit: name could simply be requiredEndpoints as it should be pretty clear we are testing the other side, not ourself
There was a problem hiding this comment.
on that note .. using a map, where you actually expect very specific fields seems odd .. schema it
data class RequiredEndpoints(
val receivers: List<ModuleId>,
val senders: List<ModuleId>,
)
| import com.izivia.ocpi.toolkit.modules.versions.domain.ModuleID | ||
|
|
||
| class CredentialsCommon { | ||
| companion object { |
There was a problem hiding this comment.
nit: I have a feeling this was code written in Java, and automatically transformed to Kotlin ... there's barely ever a need for companion objects ... just move the method out of class ... function will be part of package
There was a problem hiding this comment.
I'm pretty new to Kotlin, so I'll make more mistakes like this 😓
There was a problem hiding this comment.
I'm by no means an expert ... it looked very much like the code I wrote 3 months ago ;)
That's why I think PR comments are a great way to transmit this info, as it gives real world examples. I hope this didn't discouraged you ... overall great contribution. Also .. I wouldn't call it a mistake (ergo the nit:), just room for improvement ;)
There was a problem hiding this comment.
I'm always open to learn.
| .let { | ||
| it ?: throw OcpiServerNoMatchingEndpointsException( | ||
| "${ModuleID.credentials} other part endpoint missing" | ||
| ) | ||
| } |
There was a problem hiding this comment.
I don't think you need the let ... just react to the null
| .let { | |
| it ?: throw OcpiServerNoMatchingEndpointsException( | |
| "${ModuleID.credentials} other part endpoint missing" | |
| ) | |
| } | |
| ?: throw OcpiServerNoMatchingEndpointsException( | |
| "${ModuleID.credentials} other part endpoint missing" | |
| ) | |
| ``` |
|
Comments fixed in b29ef05 |
lilgallon
changed the title
lilgallon
force-pushed
the
feature/54_validate_required_endpoints
branch
from
b29ef05
to
319816e
Compare
|
Solves #54