-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Part 3 - applying file URL or path improvement #17149
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
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
About
Also @vunguyenhung , there are some components in this PR that were not included in the test report:
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/todoist/actions/export-tasks/export-tasks.mjs (1)
1-42
: Consider adopting the standardized file handling approach.This component still uses direct
fs
operations andtmp-promise
, which appears inconsistent with the PR's objective of standardizing file handling using@pipedream/platform
helpers likegetFileStream
andgetFileStreamAndMetadata
. Consider refactoring to align with the overall modernization effort.For consistency with the PR's file handling standardization, consider updating this component to use
@pipedream/platform
helpers:-import fs from "fs"; -import { file } from "tmp-promise"; +import { getFileStream } from "@pipedream/platform";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
components/todoist/actions/export-tasks/export-tasks.mjs
(2 hunks)components/todoist/actions/import-tasks/import-tasks.mjs
(2 hunks)components/todoist/package.json
(2 hunks)components/zoho_bugtracker/actions/create-bug/create-bug.mjs
(2 hunks)components/zoho_bugtracker/actions/update-bug/update-bug.mjs
(3 hunks)components/zoho_bugtracker/common/utils.mjs
(0 hunks)
💤 Files with no reviewable changes (1)
- components/zoho_bugtracker/common/utils.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- components/todoist/actions/import-tasks/import-tasks.mjs
- components/zoho_bugtracker/actions/update-bug/update-bug.mjs
- components/zoho_bugtracker/actions/create-bug/create-bug.mjs
- components/todoist/package.json
🔇 Additional comments (2)
components/todoist/actions/export-tasks/export-tasks.mjs (2)
10-10
: Version bump is appropriate for the functional change.The minor version increment from "0.0.3" to "0.1.0" is appropriate given the change from asynchronous to synchronous CSV conversion.
30-30
: ```shell
#!/bin/bashVerify json-2-csv dependency version and how it’s imported/used in the repo
1. Check if json-2-csv is listed in package.json and its version
grep -R ""json-2-csv"" -n package.json || echo "json-2-csv not found in package.json"
echo
2. Locate import or require statements for json-2-csv
rg "import .*json-2-csv" -n
rg "require('json-2-csv')" -n .echo
3. Search for any references to the async API
rg "json2csvAsync" -n .
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
I've created the follow-up issues as we've discussed, linked them to this PR, and prioritized them. |
/approve |
Hi everyone, all test cases are passed! Ready for release! |
Partially addresses #16977
Summary by CodeRabbit
New Features
/tmp
file paths, offering greater flexibility for file inputs.Bug Fixes
Documentation
Refactor
Chores