-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add Dropbox connector #956
base: main
Are you sure you want to change the base?
Conversation
goldflag
commented
Jan 17, 2024
•
edited
edited
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
a18b572
to
8ef9720
Compare
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.
Great work! Some minor additions needed to support a wider set of file types and handling file updates of the same file.
self.dropbox_client.sharing_create_shared_link_with_settings(path) | ||
) | ||
return link_metadata.url | ||
except ApiError as err: |
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 will fail on the same doc if the doc is updated and an update is fetched via Danswer. The error is something like: CreateSharedLinkWithSettingsError('shared_link_already_exists') so we should first check if a shared link exists?
downloaded_file = self._download_file(entry.path_display) | ||
link = self._get_shared_link(entry.path_display) | ||
try: | ||
text = downloaded_file.decode("utf-8") |
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.
We want to read the file extension and use different handling based on that. We can just assume that .pdf, .docx, .doc etc all follow their expected formats. There's another PR out right now which handles a few of these types, perhaps you can refactor their file handling code to cover these cases as well. Users will be upset if they end up missing a lot of files which they expect to get pulled in.
The important ones would be Doc, Docx, Txt, Excel, PDF, PPT. No need to address this yet though, let me review and merge the other PR first
) | ||
) | ||
except Exception as e: | ||
print( |
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.
Here and in other places - We try not to use print anywhere, we have a standard logger that we import and use throughout
@@ -0,0 +1,202 @@ | |||
"use client"; |
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.
I'm not well versed in frontend code but I don't think you need a review from Chris on this, the functionality is all good so I think the frontend code is safe to go in as is