nacx commented on this pull request.
Just couple comments I've spotted. It looks great.
Thanks for all your work on this!
> super(builder);
}
public static Properties defaultProperties() {
Properties properties =
DimensionDataCloudControlApiMetadata.defaultProperties();
+ properties.setProperty(PROPERTY_REGIONS, "na,eu,au,mea,ap,canada");
+ properties.setProperty(PROPERTY_ISO3166_CODES, "NA,EU,AU,AF,AP,CA");
Since the regions expand several countries I think we can't really put a proper
ISO code for them (like "US", "DE", etc). I'd remove the ISO code properties
for the regions and just leave them for the zones. Makes sense?
> + * 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.options;
+
+import org.jclouds.http.options.BaseHttpRequestOptions;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class DatacenterIdListFilters extends BaseHttpRequestOptions {
+
+ private DatacenterIdListFilters() {
+ }
+
+ private DatacenterIdListFilters datacenterIds(final String...
datacenterIds) {
Make this public, otherwise, if you use the builder to configure the pagination
first you won't be able to configure the ids.
Also consider providing the method that accepts a collection of IDs too, as
that's convenient since the APIs usually return collections. In my previous
comment, I pretended to suggest to "add" the varargs method, not to replace the
current one. If you add ti back, consider using `Collection` in the signature
instead of `Set`.
> + * 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.options;
+
+import org.jclouds.http.options.BaseHttpRequestOptions;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class IdListFilters extends BaseHttpRequestOptions {
+
+ private IdListFilters() {
+ }
+
+ private IdListFilters ids(final String... ids) {
Same comments here.
--
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/433#pullrequestreview-113582156
