jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Josef Cacek <notificati...@github.com>
Subject Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in MiscApi (#205)
Date Fri, 02 Oct 2015 06:58:06 GMT
@nacx, thank you for the review.

I'll rewrite the builder with the immutable `List` implemented according to sample you've
linked.

I would keep the builders in the new domain classes. IMO they improve code readability. Even
it's more talkative, it's clearer to use 
```java
ExecStartParams.builder().detach(false).build();
```
than
```java
ExecStartParams.create(false);
```

It's also simpler to keep API backward compatible with the builders. E.g. if somebody will
add [tty attribute](https://docs.docker.com/reference/api/docker_remote_api_v1.16/#exec-create)
implementation to `ExecCreateParams` class, then instead of introducing new argument of the
`create()` method, one new method to Builder's flow API will be added.

The reason why the builders were not included in all domain classes was the fact that some
of them are not intended to be used by users directly. They are used only as a return values
from remote API calls.

I'll add the fixes as a new commit and if it's OK, I can squash it into one after the review.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-144938981
Mime
View raw message