-
Notifications
You must be signed in to change notification settings - Fork 4
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
Mime type for contracts #39
Conversation
d9d0876
to
c7bf677
Compare
Pull Request Test Coverage Report for Build 120
💛 - Coveralls |
form.getCapabilities(), | ||
form.getExpectations() | ||
map(form.getCapabilities()), | ||
form.getExpectations().entrySet().stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please extract this to method? it's not easy to understand what's happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was my initial plan was to migrate to the final approach ant then get rid of any non-domain logic in ServiceContracts. For now we have there for example mapping from incoming data which binds us to non-domain logic.
If you really want it, I can do the final refactor now, but it would require to refactor duplicated code as well.
What do you think?
provider, | ||
version, | ||
mapToEntity(form.getCapabilities()), | ||
form.getExpectations().entrySet().stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please extract this to method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was my initial plan was to migrate to the final approach ant then get rid of any non-domain logic in ServiceContracts. For now we have there for example mapping from incoming data which binds us to non-domain logic.
If you really want it, I can do the final refactor now, but it would require to refactor duplicated code as well.
What do you think?
@@ -54,7 +85,7 @@ public ServiceContractsDto create(@PathVariable(name = "provider") String provid | |||
} | |||
|
|||
@GetMapping(value = "/contracts/{serviceName}", produces = MediaType.APPLICATION_JSON_VALUE) | |||
@ApiOperation(value = "Get versions of a service with registered contracts", nickname = "get versions of a service") | |||
@ApiOperation(value = "Get versions of a service with registered contracts", nickname = "create versions of a service") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this get -> create
) | ||
)); | ||
} | ||
|
||
@GetMapping(value = "/contracts", produces = MediaType.APPLICATION_JSON_VALUE) | ||
@ApiOperation(value = "Get names of services with registered contracts", nickname = "get names of services") | ||
@ApiOperation(value = "Get names of services with registered contracts", nickname = "create names of services") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert get -> create
@GetMapping(value = "/contracts/{provider}/{version:.+}", produces = MediaType.APPLICATION_JSON_VALUE) | ||
@ApiOperation(value = "Register contracts for a version of a service", nickname = "register contracts") | ||
@ApiOperation(value = "Get contracts for a version of a service", nickname = "register contracts") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really registerting contracts? (nickname)
dto.setName(serviceContracts.getName()); | ||
dto.setVersion(serviceContracts.getVersion()); | ||
@GetMapping(value = "/new/contracts/{provider}/{version:.+}", produces = MediaType.APPLICATION_JSON_VALUE) | ||
@ApiOperation(value = "Get contracts for a version of a service", nickname = "register contracts") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, is it registering contracts?
@ApiResponse(code = 200, message = "Success", response = ServiceContractsDto.class), | ||
@ApiResponse(code = 400, message = "Bad Request"), | ||
@ApiResponse(code = 500, message = "Failure")}) | ||
public NewServiceContractsDto newGet(@PathVariable(name = "provider") String provider, @PathVariable(name = "version") String version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not the best name for any method
@ApiResponses(value = { | ||
@ApiResponse(code = 200, message = "Success", response = ServiceContractsDto.class), | ||
@ApiResponse(code = 400, message = "Bad Request"), | ||
@ApiResponse(code = 500, message = "Failure")}) | ||
public ServiceContractsDto create(@PathVariable(name = "provider") String provider, @PathVariable(name = "version") String version) { | ||
public ServiceContractsDto get(@PathVariable(name = "provider") String provider, @PathVariable(name = "version") String version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getContractsForServiceInVersion
?
|
||
private Map<String, Map<String, NewServiceContractsDto.ContractDto>> mapExpectationsToDto(Map<ServiceContracts.ProviderProtocol, ServiceContracts.Contract> expectations) { | ||
HashMap<String, Map<String, NewServiceContractsDto.ContractDto>> result = newHashMap(); | ||
for (Map.Entry<ServiceContracts.ProviderProtocol, ServiceContracts.Contract> e : expectations.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loop is pretty complicated, is there any way to make it look easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least extract method with a name that will explain this logic
No description provided.