nacx requested changes on this pull request.
Some general comments:
* Prefer primitive types when non-nullable.
* Try to avoid null collections; initialize to empty.
* Avoid inheritance as Google Auto does not really support it.
* Declare the fields in the same order they appear in the factory method.
Thanks!
> + * 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;
+
+public abstract class AbstractBaseController {
+
+ public abstract String id();
+
+ public abstract String adapterType();
+
+ public abstract Integer virtualControllerId();
+
+ public abstract Integer key();
If these fields are nullable, annotate them accordingly. Otherwise prefer
primitive types.
> + * 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 org.jclouds.javax.annotation.Nullable;
+
+public abstract class AbstractDrive {
+
+ public abstract String id();
+
+ @Nullable
+ public abstract Integer sizeGb();
+
+ public abstract String state();
Is there a fixed set of values we could extract into an enum?
> + * 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 Floppy {
+
+ public abstract String id();
+
+ public abstract Integer driveNumber();
+
+ public abstract Integer virtualFloppyId();
Annotate nullable or prefer primitives.
> +package org.jclouds.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class IdeController extends AbstractBaseController {
+
+ public abstract Integer channel();
+
+ @Nullable
+ public abstract List<AbstractDrive> deviceOrDisks();
In general, we should prefer non-nullable collections. Can we default to an
empty list in the constructor/builder methods when the value comes `null` from
the server, or is there a mandatory reason to leave it null?
> + * 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 com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class IdeController extends AbstractBaseController {
Inheritance with Google auto value is not supported. You already have to define
the parent class fields in the child class factory methods and builders, wo
there is little gain abstracting the common accessors in the base class. There
are just a few, so consider removing the base calss and putting all information
here.
Also, inheritance is tricky: we annotate the "factory methods" with the json
field names, not the individual fields. This is good for deserialization but
when serializing we need to match each object field to its json name. Ideally
we would annotate each field method, but that would mean duplicating the
information from the factory method annotation. Instead, assuming the fact that
Google Auto does not support inheritance, in jclouds we took the convention
that object fields (well, abstract field accessors) would be decalred *in the
same order* than the one of the factory method, and jclouds uses that
convention when serializing. When there is a class hierarchy, field order is
not that clear and more error-prone.
> + * 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 com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class IdeController extends AbstractBaseController {
Just seeing the classes below, you need to configure the order of the
parameters in the factory methods matching the order in which the fields are
declared. Double check that for all classes.
> @@ -61,7 +61,7 @@
@RequestFilters({ BasicAuthentication.class, OrganisationIdFilter.class })
@Consumes(MediaType.APPLICATION_JSON)
-@Path("/{jclouds.api-version}/server")
+@Path("/2.6/server")
Change the default version in the API metadata instead of doing this. This way,
if users want to use the old path they can just override the property when
creating the context. All new fields in the Server object are optional, so that
shouldn't be an issue.
--
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/434#pullrequestreview-112684188
