[:coffin:] Refactoring return values#1209
Conversation
Plus it's async, but can't use it as such here
JJ
changed the title
refreshFiles is never used
fhd
left a comment
fhd
left a comment
There was a problem hiding this comment.
Think I found a few issues - but probably @RichardSto should also take a look.
| } | ||
| for (const file of storage.files) { | ||
| resolvedFiles.push(await file); | ||
| resolvedFiles.push(file); |
There was a problem hiding this comment.
It's called resolvedFiles, and the await is what resolves them. Are you sure this can go?
That said, a more succinct way of doing this would be:
const resolvedFiles = await Promise.all(storage.files);
There was a problem hiding this comment.
In that case, I think I'd also change files to filePromises or something like that. It was not entirely clear from the context that was the case.
There was a problem hiding this comment.
OK, stat returns a Promise<Stats>. That should have been obvious. Still, naming matters...
There was a problem hiding this comment.
I guess pendingFiles would work, though I personally find it obvious enough.
| const handleRemoveFile = (fileID) => { | ||
| setAccount(null); | ||
| return storage.removeFile(fileID); | ||
| storage.removeFile(fileID); |
There was a problem hiding this comment.
It appears that return value is actually being used, at least in facebookImport. I didn't check, but I'll just resume storage.removeFile is either async or explicitly returns a promise. So this function explicitly returns said promise so that it can be awaited.
A possibly more clear/modern way of writing this would be:
const handelRemoveFile = async (fileID) => {
setAccount(null);
await storage.removeFile(fileID);
};
# ✍️ Description Assigning a value to an attribute violated encapsulation, which is easy to do in JS, but still. This is a cleaner API, with a private `changeListener` which is assigned when the object is built. ### 🏗️ Works with PROD4POD-1931 Still cleaning up and refactoring the importer, looking at improving coverage. Related to #1209 and #1204, but wanted it to be considered on its own. Also it's a change of API, which is better if considered on its own.
Also ♻️ for consistency
2 participants
✍️ Description
While doing #1204 a bit of refactoring is needed; and I realized this return value is never used. Giving it its own PR since it's a change in API, and want it to be tested with the rest.
Also, some refactoring mainly eliminating
awaitandreturnwhere they're not actually needed.🏗️ Fixes PROD4PROD-1931
It contributes to cleaning the repository, and improve test coverage in
importer; eliminating code that's not used will no doubt contribute to that.