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

Error codes #103

Open
lexoyo opened this issue Sep 26, 2017 · 10 comments
Open

Error codes #103

lexoyo opened this issue Sep 26, 2017 · 10 comments

Comments

@lexoyo
Copy link
Member

lexoyo commented Sep 26, 2017

Following our discussion: unifile should return error status (not logged in, 404, server error...) + the text which is currently returned

If you give me instructions on how to do this I probably will

@JbIPS
Copy link
Collaborator

JbIPS commented Sep 26, 2017

What I had in mind:

  • Create a new error class: class UnifileError extends Error
  • Add an error code to it (maybe stick with standard HTTP code, 401 if not logged in, 500 by default and so on)
  • Replace error string by throw new UnifileError(401, "You must be logged in")

What do you say?

@lexoyo
Copy link
Member Author

lexoyo commented Sep 27, 2017

Great! It would cover my use case
I'll see if I can do this soon

@lexoyo
Copy link
Member Author

lexoyo commented Sep 27, 2017

If we follow your philosophy we should also mimic nodejs API for errors
https://nodejs.org/api/errors.html#errors_common_system_errors

lexoyo added a commit to lexoyo/unifile that referenced this issue Sep 28, 2017
lexoyo added a commit to lexoyo/unifile that referenced this issue Oct 20, 2017
lexoyo added a commit that referenced this issue Oct 20, 2017
* following the code review

* custom errors for github service
wip for #103

* fix dropbox error messages are buffers
@lexoyo
Copy link
Member Author

lexoyo commented Oct 28, 2017

With fs, when one try to delete a non-empty folder it throws this error: "errno":-39,"code":"ENOTEMPTY","syscall":"rmdir","path":"/home/lexoyo/tmp/publish"

Should dropbox service do the same?

@lexoyo
Copy link
Member Author

lexoyo commented Oct 28, 2017

Or maybe fs should remove the folder recursively

@JbIPS
Copy link
Collaborator

JbIPS commented Oct 29, 2017

That's a difficult question since I'm mot very fond of fs API, but it's based on Unix so we should probably follow it.

On the other hand, I think connectors should implement a recursiveDelete()-ish function.

JbIPS pushed a commit that referenced this issue Nov 16, 2017
* following the code review

* custom errors for github service
wip for #103

* fix dropbox error messages are buffers
@lexoyo
Copy link
Member Author

lexoyo commented Nov 23, 2017

Can you confirm @JbIPS that is missing for most connectors (all but github)

@JbIPS
Copy link
Collaborator

JbIPS commented Nov 23, 2017

I'm not sure of what you're saying but

recursive delete like fs does

fs doesn't do that, github do and I think dropbox too (to be tested)

use UnifileError and BatchError to throw errors (e.g. "path not found")

I'm implementing that in dropbox right now

@lexoyo
Copy link
Member Author

lexoyo commented Nov 24, 2017

fs doesn't do that

My bad
Then I think we should not stick to fs for this behavior and make it recursive for every service, what do you think? Or add the recursiveDelete method as you suggested?

@JbIPS
Copy link
Collaborator

JbIPS commented Nov 24, 2017

I figured out yesterday it would be very difficult to constrain GitHub and Dropbox to not remove a non-empty folder. So, for coherence sake, maybe Fs should do it too.

Maybe we should have a 'compatibility mode' (default to false) in fs constructor that would enforce that. When false the rm would remove an entire folder empty or not.

Would that be OK with everyone?

@JbIPS JbIPS mentioned this issue Nov 24, 2017
JbIPS added a commit that referenced this issue Nov 24, 2017
* Adds UT on Dropbox
* Fixes error handling
* Uses /create_folder_v2 and /dowload_v2
* Retrieve account when providing only the token
* Fixes batch upload (Closes ##114)
* Add some security check
* Normalize errors (as for #103)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants