-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(actions): add DomainNameResolver for human-friendly domain names #14355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Adds DomainNameResolver class to resolve domain URNs to human-readable display names in datahub-actions messages, improving user experience by showing friendly domain names instead of technical identifiers. Resolves: acryldata/datahub-actions#65 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Thanks for raising the PR! @hsheth2 could you take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks reasonable - can we add some tests?
Adds 10 unit test cases covering DomainNameResolver functionality: - Tests with/without DataHubGraph scenarios - Validates fallback behavior for missing/empty domain properties - Verifies NameResolverRegistry integration - Tests edge cases including None and empty string names - Ensures proper mocking and API contract verification All tests pass and follow existing code patterns and conventions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Fixes type error where DomainPropertiesClass constructor was passed None for name parameter. Uses Mock object with name attribute set to None instead to satisfy type checking. All tests continue to pass and mypy reports success with no issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests look extremely verbose - I was expecting something more like ~50 lines of code for the tests
Adds DomainNameResolver class to resolve domain URNs to human-readable display names in datahub-actions messages, improving user experience by showing friendly domain names instead of technical identifiers.
Resolves: acryldata/datahub-actions#65