-
Notifications
You must be signed in to change notification settings - Fork 8.2k
fix: add condition to not make folder download fail when flow has Note component #10953
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 base/target branches other than the default branch. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (40.02%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.7.0 #10953 +/- ##
=================================================
+ Coverage 33.05% 33.06% +0.01%
=================================================
Files 1368 1368
Lines 63815 63815
Branches 9391 9391
=================================================
+ Hits 21093 21101 +8
+ Misses 41679 41671 -8
Partials 1043 1043
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
|
||
|
|
||
| def get_cache_service() -> Union[CacheService, AsyncBaseCacheService]: | ||
| def get_cache_service() -> CacheService | AsyncBaseCacheService: |
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.
@Cristhianzl I know you made this change to fix fastapi's annotation issues could you take a look at this PR?
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.
yes, we need this Union statement.
could you please add it back and add #noqa so the ruff style doesn't change?
|
other then the 1 uinion vs pipe issue related to fastAPI I think this LGTM. But I would need someone with more expereince to look. from my understanding | and Union are the same thing |
|
yes, but it was failing in python 3.10 build for some reason. |
… to use Union for better clarity and compatibility
This pull request makes a minor fix to the logic for removing API keys from flow templates. The change ensures that the code safely checks for the existence of the
"name"key before accessing it, preventing potential errors if the key is missing.remove_api_keysby adding a check for the"name"key before using it in the API terms check.