Conversation
|
Agreed with the intent of this PR. However, there is an issue with the style. Please see inline comments. |
TKIPisalegacycipher
left a comment
TKIPisalegacycipher
left a comment
There was a problem hiding this comment.
Thank you for this submission! Please address the changes to continue the PR consideration.
| 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.
| - 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.
| 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.
| 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.
| MERAKI_PYTHON_SDK_CALLER = "" | ||
|
|
||
| # Enable or disable kwargs validation | ||
| ENABLE_KWARGS_VALIDATION = False |
There was a problem hiding this comment.
This is necessary.
| # 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.
2 participants
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.