Code Review: File IO

[jjant]

Description

I reviewed the file-io.js and its uses in main.js, no major issues were found, but several changes were made:

• There were a few uses of sync functions, these unnecessarily block the thread I've changed all of them to their async variants

• Removed unused argument legacyName of saveLegacyFolderAs

  • Changed most declarations: Removed all var uses, changing them for const or let as needed, var has strange scope rules, so the newer keywords provide saner semantics. Also, most uses of let were changed for const. This makes it clearer which variables are mutated, surprisingly only let 1 remained

• The above change highlighted a possible problem in saveSwapFolder function, where the "not savedBefore" branch returns undefined. I've added a comment noting this

• There were some unnecessary uses of promisify for fs-extra functions, those are already promisified by default (specifically, readFile and deleteFile)

• I change documentation to use JSDoc, as well as adding JSDoc type annotations, while this by itself doesn't type-check expressions, it gives handy IDE hints (for example, it makes VSCode have a much more intelligent auto-complete). This would also help if you ever decide to use TypeScript, a note about this is that Promises are not typed on their error, so typings like Promise<String, Error> should be just Promise<String>. I've decided to keep the former annotation for clarity

• Lastly, I've streamlined styling, removing inconsistent spacing, adding semi-colons were missing, etc

---

On error handling

The most important things in these two files are that the file-io.js function all had redundant, unnecessary try/catch clauses that looked like this:

function f() {
  try {
    functionThatMayThrow();
  } catch (error) {
    throw error;
  }
}

This is effectively the same as just:

function f() {
  functionThatMayThrow();
}

So I removed most of those. If you wanted to handle these errors, probably a better place to do it, is at the call site, I've added an example in main.js line 551.

[AdrianoFerrari]

Thanks @jjant ! Very thorough code review, I learned a great deal.

After testing them, I merged almost all your changes as is for file-io.js :+1:

As for the merge conflicts in main.js, I kept many of them as they were in my branch, because most of the changes there were stylistic, and I had made some breaking changes of my own in the meantime. Didn't want to mess with it too much at this point...

Again, many thanks! :smile: