search

jupyter / dashboards_server

[RETIRED] Server that runs and renders Jupyter notebooks as interactive dashboards
Other
181 stars 48 forks source link

Allows read only NOTEBOOKS_DIR v2 #272

Closed bodri5 closed 8 years ago

bodri5 commented 8 years ago

Revised version of #268

jhpedemonte commented 8 years ago

Assuming we still want to attempt to create the directory if it doesn't exist, then the patch needs some work. I'll add comments inline.

bodri5 commented 8 years ago

I fixed the indentations.

I do not quite understand @jhpedemonte's other comment. My code works the following way: first stat the path with fs.statSync. If it does not exist, then fs.statSync throws an exception, and only then we try to create the directory with fs.mkdirSync. As we are already sure, that it does not exists, any exception from mkdir can abort the program unconditionally.

jhpedemonte commented 8 years ago

@bodri5: My suggested code change was meant to address two things:

  1. I prefer to be explicit about what the code is doing. fs.statSync can throw many exceptions. The only one we want to handle is ENOENT (does not exist). All other exceptions should abort the program (hence the additional throw e in the else case).
  2. any exception from mkdir can abort the program unconditionally -> This is true and also handled in my code sample above. I just wanted to explicitly write out an error string that is more understandable to the user (the console.error line).

Make sense?

bodri5 commented 8 years ago

@jhpedemonte: Thank you for the explanation. Modified the patch according to your suggestions.

jhpedemonte commented 8 years ago

Travis was having issues, but now this is passing. I tested locally as well, pointing NOTEBOOKS_DIR at legitimate directories, a file and dir to which I didn't have permissions. All worked as expected. Merging.

Thanks @bodri5