nacx requested changes on this pull request.
Thanks @trevorflanagan, this is a great start! It looks very good. Some general
comments:
* Prefer primitives for non-nullable types. It makes the API easier to use (and
Google Auto would fail anyway if the fields are not annotated as `@Nullable`).
* Enforce immutability for collections and maps in all builders.
* Prefer using `Date` objects for all Strings that represent dates.
* Review all properties that are lists, as they name in the constructor
annotations is in "singular". Is that correct?
Thanks! Look forward to having the Dimension Data provider in!
> + public abstract List<Property> properties();
+
+ @SerializedNames({ "type", "maintenanceStatus", "property" })
+ public static Backup create(String type, String maintenanceStatus,
List<Property> properties) {
+ return
builder().type(type).maintenanceStatus(maintenanceStatus).properties(properties).build();
+ }
+
+ public abstract Builder toBuilder();
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder type(String type);
+
+ public abstract Builder maintenanceStatus(String maintenanceStatus);
+
+ public abstract Builder properties(List<Property> properties);
Use [this
pattern](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/AvailabilitySet.java#L75-L92)
to enforce immutability in collections generated by the builders. The idea is
to provide a hidden "autovalue build()" method and hidden accessors, so you can
implement the "builder.build()" in a way that enforces immutability.
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class CPU {
+
+ CPU() {
+ }
+
+ public abstract Integer count();
Better change to a primitive if non-nullable.
> +package org.jclouds.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class CPU {
+
+ CPU() {
+ }
+
+ public abstract Integer count();
+
+ public abstract String speed();
+
+ public abstract Integer coresPerSocket();
Same comment about primitives. apply this pattern to the rest of domain classes.
> +
+ @Nullable
+ public abstract List<Property> properties();
+
+ public abstract String maintenanceStatus();
+
+ @SerializedNames({ "property", "maintenanceStatus" })
+ public static ConsoleAccess create(List<Property> properties, String
maintenanceStatus) {
+ return
builder().properties(properties).maintenanceStatus(maintenanceStatus).build();
+ }
+
+ public abstract Builder toBuilder();
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder properties(List<Property> properties);
Apply the immutability pattern here too and in all other builders.
> +import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class ConsoleAccess {
+
+ ConsoleAccess() {
+ }
+
+ @Nullable
+ public abstract List<Property> properties();
+
+ public abstract String maintenanceStatus();
+
+ @SerializedNames({ "property", "maintenanceStatus" })
Shouldn't it be `properties`?
> +import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class Monitoring {
+
+ Monitoring() {
+ }
+
+ @Nullable
+ public abstract List<Property> properties();
+
+ public abstract String maintenanceStatus();
+
+ @SerializedNames({ "property", "maintenanceStatus" })
Same comment about the property name?
> +public abstract class NatRule {
+
+ NatRule() {
+ }
+
+ public static Builder builder() {
+ return new AutoValue_NatRule.Builder();
+ }
+
+ public abstract String id();
+
+ public abstract String datacenterId();
+
+ public abstract String state();
+
+ public abstract String createTime();
Can we change the type to `Date`?
> + * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.domain;
+
+import java.beans.ConstructorProperties;
+import java.util.List;
+
+/**
+ * A collection of Datacenter
+ */
+public class Datacenters extends PaginatedCollection<Datacenter> {
+
+ @ConstructorProperties({ "datacenter", "pageNumber", "pageCount",
"totalCount", "pageSize" })
Is it `datacenter`, in singular? If not, take it into account in all other
paginated collection subclasses.
> + * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class NetworkDomain {
+
+ public enum Type {
+ ESSENTIALS("ESSENTIALS"), ADVANCED("ADVANCED");
There is no need for the text attribute. Let's just use the enum's name.
> + public abstract String name();
+
+ @Nullable
+ public abstract String description();
+
+ @Nullable
+ public abstract String state();
+
+ @Nullable
+ public abstract Type type();
+
+ @Nullable
+ public abstract String snatIpv4Address();
+
+ @Nullable
+ public abstract String createTime();
Date?
> + }
+
+ public int getPageSize() {
+ return pageSize;
+ }
+
+ @Override
+ public Iterator<T> iterator() {
+ return resources.iterator();
+ }
+
+ @Override
+ public Optional<Object> nextMarker() {
+ if (totalCount < pageSize)
+ return Optional.absent();
+ if (pageNumber < (totalCount / pageSize)) {
Does the `pageNumber` start at 1 or 0? If it starts at one, this conditional
could be wrong: pageNumber=1, totalCount=7,pageSize=5 would return false, but
there is still a next page.
> + * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class ResultType {
+ ResultType() {
+ }
No properties?
> + * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class Status {
+
+ public enum ResultType {
+ ERROR("ERROR"), SUCCESS("SUCCESS"), WARNING("WARNING");
No need for the text property.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/369#pullrequestreview-27692740
