fix(typespec,agents-api): Update metadata_filter to have object type #586
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Diwank Singh Tomer <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 9decd1f in 30 seconds
More details
- Looked at
272lines of code in6files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. agents-api/agents_api/dependencies/query_filter.py:12
- Draft comment:
Theconvert_valuefunction only attempts to convert strings tointorfloat. Consider adding support forbooleanandnullconversions 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_filteris notNonebefore callingmodel_dumpto 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.
👍 Looks good to me! Incremental review on 28f66b7 in 7 seconds
More details
- Looked at
58lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted 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_filtercheck andmodel_dumpcall to avoid code repetition. This pattern is repeated inlist_docs.pyandlist_sessions.py. - Reason this comment was not posted:
Confidence changes required:50%
The code changes in multiple files involve checking ifmetadata_filteris 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.
👍 Looks good to me! Incremental review on 1f501bd in 21 seconds
More details
- Looked at
104lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/models/docs/list_docs.py:113
- Draft comment:
Themetadata_filterparameter is not used in thelist_docsfunction. 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_filteroptional 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_filteroptional 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.
👍 Looks good to me! Incremental review on 59f2423 in 16 seconds
More details
- Looked at
47lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:1319
- Draft comment:
Themetadata_filterparameter 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_filteris 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.
👍 Looks good to me! Incremental review on 71da38e in 15 seconds
More details
- Looked at
190lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. agents-api/agents_api/routers/agents/list_agents.py:20
- Draft comment:
Usedict[str, Any]instead ofdictfor better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdictas 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 ofdictfor better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdictas 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 ofdictfor better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdictas 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 ofdictfor better type hinting. - Reason this comment was not posted:
Confidence changes required:50%
The use ofdictas 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.
👍 Looks good to me! Incremental review on 63eda8b in 19 seconds
More details
- Looked at
209lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:181
- Draft comment:
The change fromresultstoitemsis 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Diwank Singh Tomer [email protected]
Important
Refactors metadata filtering by introducing
FilterModelandcreate_filter_extractor, updating API endpoints and typespec for structured metadata filtering.FilterModelandcreate_filter_extractorinquery_filter.pyfor structured metadata filtering.list_agents,list_user_docs,list_agent_docs, andlist_sessionsto useFilterModelformetadata_filter.metadata_filterinlist_agents.py,list_docs.py, andlist_sessions.py.concreteTypealias inscalars.tsp.MetadataFilteralias intypes.tspto useconcreteType.metadata_filterinPaginationOptionsmodel intypes.tspto useMetadataFilter.This description was created by
for 63eda8b. It will automatically update as commits are pushed.