Skip to content
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

fixed Command.get_argument_type bug with UnionType #110

Merged
merged 2 commits into from
May 30, 2024

Conversation

nkvuong
Copy link
Contributor

@nkvuong nkvuong commented May 30, 2024

Command.get_argument_type currently crashes when UnionType is encountered. Add special handling for this type

Copy link
Contributor

@sundarshankar89 sundarshankar89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nfx nfx self-requested a review May 30, 2024 16:06
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.00%. Comparing base (be215f1) to head (6c22e49).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   78.95%   79.00%   +0.05%     
==========================================
  Files          14       14              
  Lines        1511     1515       +4     
  Branches      273      274       +1     
==========================================
+ Hits         1193     1197       +4     
  Misses        230      230              
  Partials       88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nfx nfx merged commit 8300dd7 into databrickslabs:main May 30, 2024
12 of 13 checks passed
nfx added a commit that referenced this pull request May 30, 2024
* fixed `Command.get_argument_type` bug with `UnionType` ([#110](#110)). In this release, the `Command.get_argument_type` method has been updated to include special handling for `UnionType`, resolving a bug that caused the function to crash when encountering this type. The method now returns the string representation of the annotation if the argument is a `UnionType`, providing more accurate and reliable results. To facilitate this, modifications were made using the `types` module. Additionally, the `foo` function has a new optional argument `optional_arg` of type `str`, with a default value of `None`. This argument is passed to the `some` function in the assertion. The `Prompts` type has been added to the `foo` function signature, and an assertion has been added to verify if `prompts` is an instance of `Prompts`. Lastly, the default value of the `address` argument has been changed from an empty string to "default", and the same changes have been applied to the `test_injects_prompts` test function.
@nfx nfx mentioned this pull request May 30, 2024
nfx added a commit that referenced this pull request May 30, 2024
* fixed `Command.get_argument_type` bug with `UnionType`
([#110](#110)). In
this release, the `Command.get_argument_type` method has been updated to
include special handling for `UnionType`, resolving a bug that caused
the function to crash when encountering this type. The method now
returns the string representation of the annotation if the argument is a
`UnionType`, providing more accurate and reliable results. To facilitate
this, modifications were made using the `types` module. Additionally,
the `foo` function has a new optional argument `optional_arg` of type
`str`, with a default value of `None`. This argument is passed to the
`some` function in the assertion. The `Prompts` type has been added to
the `foo` function signature, and an assertion has been added to verify
if `prompts` is an instance of `Prompts`. Lastly, the default value of
the `address` argument has been changed from an empty string to
"default", and the same changes have been applied to the
`test_injects_prompts` test function.
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