-
Notifications
You must be signed in to change notification settings - Fork 5.6k
#938 - pCloud new actions and source #3240
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
andrewjschuang
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.
@GTFalcao thanks for the contribution!
Looks good but I've left a few comments below
andrewjschuang
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.
@GTFalcao thanks for the changes!
I've made a few comments about maintaining consistent indentation for the newline separator, \\, and a few extra lines.
I also still see some return await that aren't necessary in the plcoud.app.mjs file.
components/pcloud/pcloud.app.mjs
Outdated
| }; | ||
| return retry(async (bail) => { | ||
| try { | ||
| return await apiCall(); |
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.
This await is actually needed since it's inside a try/catch statement
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.
Fixed this - many thanks for the very complete reviews!
andrewjschuang
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.
Thanks for the changes! I've left one comment but moving it to QA.
|
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 |
|
Hi everyone, all test cases are passed! Ready for release! Test report |
|
/approve |
New Actions
New Source
The code used as a base pull request #1392, beginning by importing all code from Sergio's fork to this new branch.