Fix/log invalid kwarg #238
Conversation
|
Agreed with the intent of this PR. However, there is an issue with the style. Please see inline comments. |
| @@ -40,6 +40,7 @@ | |||
| BE_GEO_ID, | |||
| MERAKI_PYTHON_SDK_CALLER, | |||
| USE_ITERATOR_FOR_GET_PAGES, | |||
| ENABLE_KWARGS_VALIDATION, | |||
There was a problem hiding this comment.
This makes sense and is necessary.
| @@ -72,6 +73,7 @@ class DashboardAPI(object): | |||
| - be_geo_id (string): optional partner identifier for API usage tracking; can also be set as an environment variable BE_GEO_ID | |||
| - caller (string): optional identifier for API usage tracking; can also be set as an environment variable MERAKI_PYTHON_SDK_CALLER | |||
| - use_iterator_for_get_pages (boolean): list* methods will return an iterator with each object instead of a complete list with all items | |||
| - enable_kwarg_validation (boolean): enable kwargs validation? | |||
There was a problem hiding this comment.
This makes sense and is necessary.
| @@ -97,6 +99,7 @@ def __init__(self, | |||
| caller=MERAKI_PYTHON_SDK_CALLER, | |||
| use_iterator_for_get_pages=USE_ITERATOR_FOR_GET_PAGES, | |||
| inherit_logging_config=INHERIT_LOGGING_CONFIG, | |||
| enable_kwarg_validation=ENABLE_KWARGS_VALIDATION, | |||
There was a problem hiding this comment.
This makes sense and seems necessary.
| @@ -167,20 +170,21 @@ def __init__(self, | |||
| use_iterator_for_get_pages=use_iterator_for_get_pages, | |||
| ) | |||
|
|
|||
| self._enable_kwargs_validation = enable_kwarg_validation | |||
There was a problem hiding this comment.
This should not be needed. None of the other config kwargs receive this treatment and we should not start treating kwargs this way.
| self.administered = Administered(self._session, self._enable_kwargs_validation ) | ||
| self.organizations = Organizations(self._session, self._enable_kwargs_validation ) | ||
| self.networks = Networks(self._session, self._enable_kwargs_validation ) | ||
| self.devices = Devices(self._session, self._enable_kwargs_validation ) | ||
| self.appliance = Appliance(self._session, self._enable_kwargs_validation ) | ||
| self.camera = Camera(self._session, self._enable_kwargs_validation ) | ||
| self.cellularGateway = CellularGateway(self._session, self._enable_kwargs_validation ) | ||
| self.insight = Insight(self._session, self._enable_kwargs_validation ) | ||
| self.licensing = Licensing(self._session, self._enable_kwargs_validation ) | ||
| self.sensor = Sensor(self._session, self._enable_kwargs_validation ) | ||
| self.sm = Sm(self._session, self._enable_kwargs_validation ) | ||
| self.switch = Switch(self._session, self._enable_kwargs_validation ) | ||
| self.wireless = Wireless(self._session, self._enable_kwargs_validation ) |
There was a problem hiding this comment.
This should not be needed. None of the other config kwargs receive this treatment and we should not start treating kwargs this way.
What prompted this specific part of the PR?
| self.organizations = ActionBatchOrganizations(enable_kwarg_validation) | ||
| self.networks = ActionBatchNetworks(enable_kwarg_validation) | ||
| self.devices = ActionBatchDevices(enable_kwarg_validation) | ||
| self.appliance = ActionBatchAppliance(enable_kwarg_validation) | ||
| self.camera = ActionBatchCamera(enable_kwarg_validation) | ||
| self.cellularGateway = ActionBatchCellularGateway(enable_kwarg_validation) | ||
| self.insight = ActionBatchInsight(enable_kwarg_validation) | ||
| self.sensor = ActionBatchSensor(enable_kwarg_validation) | ||
| self.sm = ActionBatchSm(enable_kwarg_validation) | ||
| self.switch = ActionBatchSwitch(enable_kwarg_validation) | ||
| self.wireless = ActionBatchWireless(enable_kwarg_validation) |
There was a problem hiding this comment.
This should not be needed. None of the other config kwargs receive this treatment and we should not start treating kwargs this way.
| @@ -84,3 +84,6 @@ | |||
| # The choice is yours as long as you follow the format. You should **not** include other information in this string. | |||
| # If you are an official ecosystem partner, this is required. | |||
| MERAKI_PYTHON_SDK_CALLER = "" | |||
|
|
|||
| # Enable or disable kwargs validation | |||
| ENABLE_KWARGS_VALIDATION = False | |||
There was a problem hiding this comment.
This is necessary.
| @@ -13,16 +13,16 @@ | |||
|
|
|||
| # Batch class | |||
| class Batch: | |||
| def __init__(self): | |||
| def __init__(self, enable_kwarg_validation): | |||
There was a problem hiding this comment.
This should not be needed. None of the other config kwargs receive this treatment and we should not start treating kwargs this way.
|
|
||
| # Batch definitions | ||
| self.batch = Batch() | ||
| self.batch = Batch(self._enable_kwargs_validation ) |
There was a problem hiding this comment.
This should not be needed. None of the other config kwargs receive this treatment and we should not start treating kwargs this way.
Hi @TKIPisalegacycipher , thank you for your feedback. Based on your suggestion I've made new changes in this PR.
Let me know if this resolves the issue.