-
Notifications
You must be signed in to change notification settings - Fork 904
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
fix(typespec,agents-api): Update metadata_filter to have object type #586
Conversation
Signed-off-by: Diwank Singh Tomer <[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.
👍 Looks good to me! Reviewed everything up to 9decd1f in 30 seconds
More details
- Looked at
272
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. agents-api/agents_api/dependencies/query_filter.py:12
- Draft comment:
Theconvert_value
function only attempts to convert strings toint
orfloat
. Consider adding support forboolean
andnull
conversions to handle these types as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is suggesting a potential improvement to the convert_value function by adding support for boolean and null conversions. This is a valid suggestion as it could make the function more robust and versatile. The comment is actionable and clear, as it directly points out a limitation in the current implementation and suggests a way to enhance it.
The comment assumes that handling boolean and null conversions is necessary, but without context on how the function is used, it's speculative. The current implementation might be sufficient for its intended use.
Even without full context, the suggestion to handle more data types is a common enhancement for functions dealing with data conversion. It is a reasonable improvement that could prevent future issues.
The comment is a valid suggestion for a code quality improvement and should be kept as it provides a clear and actionable enhancement to the convert_value function.
2. agents-api/agents_api/routers/agents/list_agents.py:35
- Draft comment:
Ensuremetadata_filter
is notNone
before callingmodel_dump
to avoid potentialAttributeError
. This applies to similar usages inlist_user_docs
,list_agent_docs
, andlist_sessions
. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_7CfBVlS9iSaBa5iD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Singh Tomer <[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.
👍 Looks good to me! Incremental review on 28f66b7 in 7 seconds
More details
- Looked at
58
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/routers/agents/list_agents.py:33
- Draft comment:
Consider using a utility function to handle themetadata_filter
check andmodel_dump
call to avoid code repetition. This pattern is repeated inlist_docs.py
andlist_sessions.py
. - Reason this comment was not posted:
Confidence changes required:50%
The code changes in multiple files involve checking ifmetadata_filter
is not None before callingmodel_dump
. This pattern is repeated across several files.
Workflow ID: wflow_dZCTNOkq0DJxerTR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Singh Tomer <[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.
👍 Looks good to me! Incremental review on 1f501bd in 21 seconds
More details
- Looked at
104
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. agents-api/agents_api/models/docs/list_docs.py:113
- Draft comment:
Themetadata_filter
parameter is not used in thelist_docs
function. Consider removing it from the function signature if it's not needed. - Reason this comment was not posted:
Comment was on unchanged code.
2. agents-api/agents_api/routers/docs/list_docs.py:16
- Draft comment:
Consider makingmetadata_filter
optional to avoid issues if not provided. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code change shows a clear intention to make 'metadata_filter' a required parameter by removing the 'None' option. The comment contradicts this change, suggesting an optional parameter, which is not aligned with the current code. The comment does not provide strong evidence or reasoning to revert the change.
I might be missing some context about why 'metadata_filter' was made required. However, the code change itself is a strong indicator of the intended behavior.
The code change is explicit in its intention, and without additional context or evidence, the comment seems to be incorrect.
The comment should be deleted as it contradicts the explicit code change made in the diff.
3. agents-api/agents_api/routers/docs/list_docs.py:44
- Draft comment:
Consider makingmetadata_filter
optional to avoid issues if not provided. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_qk3EGOB7lQgKTdbJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Singh Tomer <[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.
👍 Looks good to me! Incremental review on 59f2423 in 16 seconds
More details
- Looked at
47
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:1319
- Draft comment:
Themetadata_filter
parameter has been correctly updated to an object type, which aligns with the PR's intent to improve structured metadata filtering. This change enhances clarity and usability. - Reason this comment was not posted:
Confidence changes required:0%
The change from a JSON string to an object formetadata_filter
is consistent with the PR description and improves clarity and usability.
Workflow ID: wflow_tGQ9V95l7tb6rGhZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Singh Tomer <[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.
👍 Looks good to me! Incremental review on 71da38e in 15 seconds
More details
- Looked at
190
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. agents-api/agents_api/routers/agents/list_agents.py:20
- Draft comment:
Usedict[str, Any]
instead ofdict
for better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdict
as a type hint is not specific enough. It should bedict[str, Any]
to indicate the expected types for keys and values.
2. agents-api/agents_api/routers/docs/list_docs.py:17
- Draft comment:
Usedict[str, Any]
instead ofdict
for better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdict
as a type hint is not specific enough. It should bedict[str, Any]
to indicate the expected types for keys and values.
3. agents-api/agents_api/routers/docs/list_docs.py:43
- Draft comment:
Usedict[str, Any]
instead ofdict
for better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdict
as a type hint is not specific enough. It should bedict[str, Any]
to indicate the expected types for keys and values.
4. agents-api/agents_api/routers/sessions/list_sessions.py:17
- Draft comment:
Usedict[str, Any]
instead ofdict
for better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdict
as a type hint is not specific enough. It should bedict[str, Any]
to indicate the expected types for keys and values.
Workflow ID: wflow_BXUksojBrzuW2A0y
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Singh Tomer <[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.
👍 Looks good to me! Incremental review on 63eda8b in 19 seconds
More details
- Looked at
209
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:181
- Draft comment:
The change fromresults
toitems
is consistent with the PR description and is applied correctly across the file. Ensure that any dependent code or documentation is updated accordingly. - Reason this comment was not posted:
Confidence changes required:20%
The change from 'results' to 'items' is consistent across the file and aligns with the PR description.
Workflow ID: wflow_dBnUZOBrgghmX6oH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
LGTM!
Signed-off-by: Diwank Singh Tomer [email protected]
Important
Refactors metadata filtering by introducing
FilterModel
andcreate_filter_extractor
, updating API endpoints and typespec for structured metadata filtering.FilterModel
andcreate_filter_extractor
inquery_filter.py
for structured metadata filtering.list_agents
,list_user_docs
,list_agent_docs
, andlist_sessions
to useFilterModel
formetadata_filter
.metadata_filter
inlist_agents.py
,list_docs.py
, andlist_sessions.py
.concreteType
alias inscalars.tsp
.MetadataFilter
alias intypes.tsp
to useconcreteType
.metadata_filter
inPaginationOptions
model intypes.tsp
to useMetadataFilter
.This description was created by for 63eda8b. It will automatically update as commits are pushed.