PipedreamHQ / pipedream

Connect APIs, remarkably fast. Free for developers.
https://pipedream.com
Other
8.32k stars 5.27k forks source link

Solving #2870, Adding parameter to filter for trashed folders #2871

Closed bsommardahl closed 1 year ago

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
pipedream-docs ✅ Ready (Inspect) Visit Preview Jun 9, 2022 at 2:16PM (UTC)
pipedream-docs-redirect-do-not-edit ✅ Ready (Inspect) Visit Preview Jun 9, 2022 at 2:16PM (UTC)
dylburger commented 2 years ago

Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:

dylburger commented 2 years ago

Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified.

dannyroosevelt commented 1 year ago

@bsommardahl do you want to wrap this up and implement the suggested changes, or do you want one of our devs to finish up the remaining work?

bsommardahl commented 1 year ago

I'll make sure it gets done tomorrow. Thanks for the nudge.

On Tue, May 24, 2022, 6:42 PM Danny Roosevelt @.***> wrote:

@bsommardahl https://github.com/bsommardahl do you want to wrap this up and implement the suggested changes, or do you want one of our devs to finish up the remaining work?

— Reply to this email directly, view it on GitHub https://github.com/PipedreamHQ/pipedream/pull/2871#issuecomment-1136536038, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2L335SFDZUPYEREBI3HLVLVSOZANCNFSM5WFAWORQ . You are receiving this because you were mentioned.Message ID: @.***>

dannyroosevelt commented 1 year ago

I'll make sure it gets done tomorrow. Thanks for the nudge. On Tue, May 24, 2022, 6:42 PM Danny Roosevelt @.> wrote: @bsommardahl https://github.com/bsommardahl do you want to wrap this up and implement the suggested changes, or do you want one of our devs to finish up the remaining work? — Reply to this email directly, view it on GitHub <#2871 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2L335SFDZUPYEREBI3HLVLVSOZANCNFSM5WFAWORQ . You are receiving this because you were mentioned.Message ID: @.>

Sounds good!

dylburger commented 1 year ago

@bsommardahl I just wanted to check in on this. Do you still want to wrap these changes up, or would it be easier for us to make the updates?

bsommardahl commented 1 year ago

Well, there was my hope and there was reality. Maybe better if you finish it up for me. Sorry, I have gotten busy again.

On Mon, Jun 6, 2022 at 6:12 PM Dylan J. Sather @.***> wrote:

@bsommardahl https://github.com/bsommardahl I just wanted to check in on this. Do you still want to wrap these changes up, or would it be easier for us to make the updates?

— Reply to this email directly, view it on GitHub https://github.com/PipedreamHQ/pipedream/pull/2871#issuecomment-1148021806, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2L37IW6IC2RMULRV5LFDVN2AV3ANCNFSM5WFAWORQ . You are receiving this because you were mentioned.Message ID: @.***>

dannyroosevelt commented 1 year ago

Well, there was my hope and there was reality. Maybe better if you finish it up for me. Sorry, I have gotten busy again. On Mon, Jun 6, 2022 at 6:12 PM Dylan J. Sather @.> wrote: @bsommardahl https://github.com/bsommardahl I just wanted to check in on this. Do you still want to wrap these changes up, or would it be easier for us to make the updates? — Reply to this email directly, view it on GitHub <#2871 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2L37IW6IC2RMULRV5LFDVN2AV3ANCNFSM5WFAWORQ . You are receiving this because you were mentioned.Message ID: @.>

No problem at all! Thank you for the original contribution!

@andrewjschuang can you implement your suggested changes?

andrewjschuang commented 1 year ago

Ready for QA

DilanAthukorala commented 1 year ago

@andrewjschuang @dannyroosevelt If we set trashed = true this will return only the folders in trash. It won't return folders that are not in trash. So if this is the behavior of the API then the name and description here saying 'include trash' is misleading. And should we set the default value as false ?

image image

andrewjschuang commented 1 year ago

@andrewjschuang @dannyroosevelt If we set trashed = true this will return only the folders in trash. It won't return folders that are not in trash. So if this is the behavior of the API then the name and description here saying 'include trash' is misleading. And should we set the default value as false ?

@DilanAthukorala So let me check if I understand correctly.

  1. If the parameter is not sent, it will return all folders including those in trash.
  2. If trashed = true, it will return only those in trash
  3. If trashed = false, it will return only those not in trash

We could keep the name/description and change the behaviour of the action.

true -> 1. false -> 3.

DilanAthukorala commented 1 year ago

@andrewjschuang @dannyroosevelt If we set trashed = true this will return only the folders in trash. It won't return folders that are not in trash. So if this is the behavior of the API then the name and description here saying 'include trash' is misleading. And should we set the default value as false ?

@DilanAthukorala So let me check if I understand correctly.

1. If the parameter is not sent, it will return all folders including those in trash.

2. If `trashed = true`, it will return only those in trash

3. If `trashed = false`, it will return only those **not** in trash

We could keep the name/description and change the behaviour of the action.

true -> 1. false -> 3.

@andrewjschuang Yes we could make trashed param optional and not send it by default. But again I think the description implies that setting trashed=true means it includes trashed folders along with the other folders (not trashed) which is not the actual behavior of that param.

DilanAthukorala commented 1 year ago

Verification completed.

dannyroosevelt commented 1 year ago

This is ready for release!

andrewjschuang commented 1 year ago

/approve