Skip to content
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

Implementing sharelink feature requested in #77 #89

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

banool
Copy link
Contributor

@banool banool commented Jan 3, 2018

This is in response to #77.

Following the current command structure for dbxcli, the new command can be found at dbxcli share getlink.

If the file / folder already has a sharelink, that link is retrieved and printed. This follows the pre-existing printing format from the sharing functions. If it doesn't have a sharelink, the link is created and printed.

There is also support for getting links for files that haven't fully uploaded yet. I wanted to functionality to be precisely like if you right click on a file and use Copy Dropbox Link. The side effect of this is I had to use the partially deprecated create_shared_link endpoint.

This is the first Go I've ever coded so I'm of course open to revisions.

@@ -0,0 +1,147 @@
// Copyright © 2017 Dropbox, Inc.
// Author: Daniel Porteous
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, can you omit this line? git blame can always reveal authors :)

"github.com/spf13/cobra"
)

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use // to start comments. While go does support C style block comments, // is the norm and Javadoc-style comments are rare. More at https://golang.org/doc/effective_go.html#commentary

// Confirm that the file exists.
exists, err := exists(path)
if !exists || err != nil {
print("The file / folder specified does not exist.\n")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print the path here as well so users have context

return
}

// print("File / folder does not yet have a sharelink, creating one...\n")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete?

fmt.Printf("Usage: %s share getlink [file / folder path]\n", os.Args[0])
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for comments here and elsewhere, start with //

** Try to get an existing share link for a file / folder.
** It returns true if the file / folder had a link. Otherwise it returns false.
*/
func getExistingLink(dbx sharing.Client, path string) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this function is counter-intuitive, as it doesn't actually return the existing link. Perhaps checkExistingLink?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't just check the existing link though, it also returns it if it exists. I think checkExistingLinkAndReturnIfExists would be a bit wordy 😉. I can change it regardless if you want.

** CreateSharedLinkWithSettings doesn't allow pending uploads,
** so we use the partially deprecated CreateSharedLink.
*/
func getNewLink(dbx sharing.Client, path string) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are using bool to handle errors. Why not return error instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea, I'll try that.

// Get the sharelink even if the file isn't fully uploaded yet.
arg.PendingUpload = new(sharing.PendingUploadMode)
// Determine whether the target is a file or folder.
fi, err := os.Stat(path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this check before creating arg

}

/* Return the path of the Dropbox folder. */
func getDropboxFolder() string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole function is super hacky and likely not cross-platform. Let me think if there's a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks. I'll address the other issues now and we can come back to this when you have a solution. There should be a function somewhere that returns where the Dropbox folder is, the information has to be held somewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the complexity around obtaining the "canonical" Dropbox folder, for all scenarios (e.g. paid users, paired accounts, custom directories) and for all platforms, I'd prefer if we solve this via documentation and help, and being explicit. For instance, we can ask users to just supply the path instead of trying to prefix things ourselves. WDYT?

@banool
Copy link
Contributor Author

banool commented Jan 11, 2018

I addressed all of the issues except those related to getDropboxFolder.

@coatless
Copy link

Any ETA on when this feature can be live?

@banool
Copy link
Contributor Author

banool commented Jan 18, 2018

@coatless Just waiting on the go ahead from @diwakergupta, as well as perhaps a cleaner way to do the functionality provided by getDropboxFolder.

@banool
Copy link
Contributor Author

banool commented Feb 25, 2018

@diwakergupta Think you could have another look at this?

@coatless
Copy link

coatless commented Mar 30, 2018

@banool @diwakergupta it would be great to see headway on this feature as the ~/dropbox.py script listed under Linux Installation appears to be at EOL given:

root@remote:~# ~/dropbox.py status
You're using an old version of Dropbox. Please update within the next 4 days to continue using Dropbox.

https://www.dropbox.com/downloading?from_client=True
Up to date

@diwakergupta
Copy link
Collaborator

Sorry folks, will take a look right away!

@MartinDelille
Copy link

@diwakergupta Any update on this PR? I'd love to use this command.

I installed dbxcli only for this purpose because my current workflow is:

  • open -r myfile
  • Right click on the file: copy shareable link

@MartinDelille
Copy link

@banool is there a way to install your version to test it?

@cahebert
Copy link

Bumping this, I would love to see this feature incorporated into the main branch.

@batt
Copy link

batt commented Nov 11, 2019

I would love to have this PR merged too!

@vlaminck
Copy link

@diwakergupta any update on if this will get merged? It would be really nice to have.

@vlaminck
Copy link

I know this is super old and seems to not actually be maintained any more, but when I try to go build your branch, I get this error:

./main.go:43:24: cannot use versionCmd (type *"github.com/banool/dbxcli/vendor/github.com/spf13/cobra".Command) as type *"github.com/dropbox/dbxcli/vendor/github.com/spf13/cobra".Command in argument to cmd.RootCmd.AddCommand

@coatless
Copy link

Sadly, I think this software is official dead. I wouldn't push forward more on this as it literally was a year and then some. Consider looking into Dropbox alternatives.

@banool
Copy link
Contributor Author

banool commented Nov 23, 2019

Unfortunately yes, I haven't looked into this PR for a long time. I don't intend to pick up the work again either since, as @coatless says, this appears to be entirely dead. Dropbox seems to be more interested in the designer / creative space than the technical space these days, so I don't expect to see any further development on this either. Given the recent price increase and worsening features relative to competitors, might be time to look elsewhere.

@vlaminck
Copy link

Yeah, I figured that'd be the case. Thanks for the responses.

If anyone finds this in the future, here's a Groovy script that I wrote to accomplish what I needed. https://gitlab.com/snippets/1915960

@MartinDelille
Copy link

I wanted to give it a look and I manage to make it work!

I rebased the PR (otherwise I had a segmentation fault).

$ dbxcli share getlink ~/Dropbox/Flyers\ Lyon.jpg
2019/11/23 19:38:45 client.go:619: WARNING: API `CreateSharedLink` is deprecated
2019/11/23 19:38:45 client.go:620: Use API `CreateSharedLinkWithSettings` instead
Flyers Lyon.jpg	https://www.dropbox.com/s/1mvz1bhiq7csq1l/Flyers%20Lyon.jpg?dl=0

As you can see, CreateSharedLink is deprecated but it shouldn't be a problem to fix that.

Regarding the output I would remove the file name in order to make the result "pipable":

$ dbxcli share getlink ~/Dropbox/Flyers\ Lyon.jpg | pbcopy

(for non MacOS user, pbcopy copy the input in the clipboard)

@biounix
Copy link

biounix commented Aug 3, 2020

To anybody interested in that option:

#113 (comment)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@banool
Copy link
Contributor Author

banool commented Apr 19, 2022

I'm unsubscribing from this, please tag me if you need me if the maintainers ever return. Thanks!

@MartinDelille
Copy link

@banool If you want to get notified, stay subscribed because I don't think I will remember to ping you when it merged (there is not so much activity here anyway).

@biounix I just tried tbx for the first time after having it under my watched repository since you mentioned it. I opened a question here about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants