Skip to content

Improve code quality - base class, exceptions, imports, tests#122

Merged
shanghaikid merged 2 commits into
zilliztech:mainfrom
cloudsmithy:main
Apr 8, 2026
Merged

Improve code quality - base class, exceptions, imports, tests#122
shanghaikid merged 2 commits into
zilliztech:mainfrom
cloudsmithy:main

Conversation

@cloudsmithy
Copy link
Copy Markdown

Summary

Code quality refactoring that reduces ~400 lines while fixing several bugs and improving maintainability.

Changes

Architecture

  • Add BaseMilvusClient base class — eliminates duplicated __init__ + _get_client() across 10 client modules
  • Add safe_int() utility — eliminates repeated null/string checking for row_count
  • Remove duplicate OutputFormatter class from CliClient.py (was defined in both CliClient.py and OutputFormatter.py)

Bug Fixes

  • show_loading_progress was returning hardcoded {"loading_progress": "100%"} — now calls real get_load_state API
  • get_index_build_progress was returning hardcoded {"progress": "100%", "state": "Finished"} — now calls real describe_index API
  • insert/upsert list-of-lists conversion had field misalignment when collection has auto_id — now correctly skips auto_id fields
  • list_user_roles could return tuple instead of list — now always returns list
  • CliClient.__init__ assigned self.formatter twice

Exception Handling

  • Replace all 93 raise Exception(f"...{str(e)}") with raise RuntimeError/ValueError(f"...{e}") from e
  • Fix all 26 bare except:except Exception: (was catching KeyboardInterrupt)
  • Use ConnectionError in BaseMilvusClient._get_client()
  • Use ValueError for business logic validation (missing partition, user not found, etc.)

Import Cleanup

  • Replace from .xxx import * with explicit side-effect imports in entry points
  • Replace sys.path.append hack with relative imports in 7 script files
  • Add try/except ImportError pattern for dual-mode compatibility (package import + direct run)
  • Remove unused import sys / import os from 7 files

Housekeeping

  • Remove stale Dockerfile.test (single line, unused)
  • Remove .DS_Store from git tracking
  • Add .cursor/, .kiro/, test.env to .gitignore
  • Update README.md project structure to match actual filenames
  • Remove test.env from repo (contained credentials)

Test Fixes

  • Fix test_describe_database — assert correct API response key (name not database_name)
  • Fix test_get_partition_stats — use _default partition to avoid ordering dependency
  • Fix test_list_users — don't hardcode db_admin (Zilliz Cloud specific)
  • Fix test_get_index_build_progress — match real API response format
  • Handle auth errors gracefully in test_grant_and_revoke_privilege
  • Fix all bare except: in test files

Stats

40 files changed, 496 insertions(+), 895 deletions(-)

Testing

102 unit tests passing against local Milvus standalone.

Summary:
- Add BaseMilvusClient base class, eliminating ~180 lines of duplicated code
- Add safe_int() utility for robust null/string handling
- Fix all 93 'raise Exception' → RuntimeError/ValueError with exception chaining
- Fix all bare 'except:' → 'except Exception:' across core and test modules
- Fix fake implementations (show_loading_progress, get_index_build_progress)
- Fix insert/upsert list-of-lists field misalignment with auto_id fields
- Remove duplicate OutputFormatter class and double formatter assignment
- Replace wildcard imports with explicit side-effect imports
- Replace sys.path.append hack with relative imports
- Add try/except ImportError for dual-mode import compatibility
- Ensure list_user_roles always returns list
- Update README/TESTING.md project structure
- Fix 6 flaky unit tests (API response format, env assumptions)
- Clean up stale Dockerfile.test, add .cursor/.kiro/test.env to .gitignore
@nameczz
Copy link
Copy Markdown
Collaborator

nameczz commented Mar 30, 2026

@cloudsmithy Thanks for the PR! It looks great, but merging is currently blocked because your commits need verified signatures.
Could you please re-sign your commits or set up a GPG key to meet the Verified Signature requirement? Once that's done, we should be able to move forward. Thanks!

@shanghaikid shanghaikid merged commit 5554eb9 into zilliztech:main Apr 8, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants