-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: add EarthScienceToolkit param types and supported formats in MarkItDownLoader #3764
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
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Pull request overview
This PR addresses issue #3763 by adding type annotations to multiple methods in EarthScienceToolkit that were missing parameter types. It also adds .md format support to MarkItDownLoader and improves the RetrievalToolkit docstring to use f-string interpolation for displaying default values dynamically.
Changes:
- Added type annotations to 15 methods in
EarthScienceToolkitto fix missing parameter types - Added
.mdformat to theSUPPORTED_FORMATSlist inMarkItDownLoader - Enhanced
RetrievalToolkitdocstring to use f-string interpolation for dynamic default value display - Added unit tests for markdown file conversion in
MarkItDownLoader
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| camel/toolkits/earth_science_toolkit.py | Added type annotations to 15 methods to ensure proper tool schema generation with type information |
| camel/loaders/markitdown.py | Added .md extension to supported formats list for markdown file processing |
| camel/toolkits/retrieval_toolkit.py | Changed docstring to use f-string formatting for dynamic constant interpolation |
| test/loaders/test_markitdown.py | Added markdown file test case and updated test assertions to include the new format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
thanks for the contributions. left some comments, for the code styles.
waleedalzarooni
left a comment
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.
Hey @fanzhidongyzby,
Thanks for the great PR, I can't say I have anything to add other than what's already been said. Just make sure to resolve any linting and docstring warnings, I will get some feedback from a more senior maintainer over what to do with the rf string issue and we can go from there!
ebb108d to
10ec846
Compare
Appointat
left a comment
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
waleedalzarooni
left a comment
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.
just one quick amendment for the use of rf
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.
No issues from myside. Thanks @fanzhidongyzby
Thanks, and I wander which version will be released including this pr, so I can update camel-ai dependency version in my project. |
Description
Fixes #3763
Changes
.mdformat support toMarkItDownLoaderEarthScienceToolkitRetrievalToolkitwith dynamic default valuesChecklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!