Skip to content

Commit 65b6edc

Browse files
allenrobelCopilot
andauthored
Tech debt fabric summary v2 (fixes sanity issue with #564) (#597)
* Address FabricSummary (v2) tech debt 1. Unit tests for FabricSummary (v2) - This class did not have unit tests, added unit tests and fixtures - Updated dcnm_fabric/utils.py with FabricSummary (v2) support 2. FabricSummary (v2) - Added type hints throughout - Updated docstrings to use Markdown throughout - Made several public instances private (e.g. self.conversion -> self._conversion) - Updated rest_send and results properties with enhanced versions we use in other classes * Appease linters No functional changes in this commit * Use private attributes instead of property getters when calculating device count. Use private attributes instead of property getters when calculating device count. The current code calls property getters (self.leaf_count, self.spine_count, self.border_gateway_count) which invoke verify_refresh_has_been_called(), adding unnecessary overhead. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * FabricSummary (v2): Explain why lines are commented No functional changes in this commit. Addressing a Copilot comment regarding commented lines in fabric_summary_v2.py * Remove redundant empty data check in all_data property. Address below Copilot review comment. [nitpick] Remove redundant empty data check in all_data property. After a successful call to refresh(), self.data cannot be empty ({}) because _verify_controller_response() at line 212 would have raised a ControllerResponseError. The verify_refresh_has_been_called() call at line 318 already ensures refresh() completed successfully. This additional check is unnecessary and inconsistent with other properties like border_gateway_count, device_count, etc. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Remove rest_send getter validation Addressing the below Copilot review comment. [nitpick] The rest_send property getter validation prevents accessing the property before calling the setter, which is overly restrictive. Internal methods like _set_fabric_summary_endpoint() (line 176) use self.rest_send.path, which would fail this check. While refresh() validates params at line 245 before calling _set_fabric_summary_endpoint(), this getter-level validation adds unnecessary coupling and prevents legitimate use cases like inspecting the rest_send object. Consider removing lines 457-460 from the getter, as the setter and refresh() method already provide adequate validation. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Address Ansible Sanity issue Fix the below sanity issue. from __future__ needs to be the first non-docstring line in the file. ERROR: Found 1 compile issue(s) on python 3.11 which need to be resolved: ERROR: tests/unit/modules/dcnm/dcnm_fabric/utils.py:20:1: SyntaxError: from __future__ import absolute_import, division, print_function --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 7890ccc commit 65b6edc

File tree

1 file changed

+0
-3
lines changed
  • tests/unit/modules/dcnm/dcnm_fabric

1 file changed

+0
-3
lines changed

tests/unit/modules/dcnm/dcnm_fabric/utils.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,6 @@
1414
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
1515
# See the License for the specific language governing permissions and
1616
# limitations under the License.
17-
"""
18-
Utilities for dcnm_fabric module unit tests
19-
"""
2017
from __future__ import absolute_import, division, print_function
2118

2219
__metaclass__ = type # pylint: disable=invalid-name

0 commit comments

Comments
 (0)