Graceful dat.json title update #537
Conversation
There was a problem hiding this comment.
I took some time and look into this and I dont think this solves our problem.
- There are many reasons why reading could fail, simply ignoring the error doesn't seem like the right approach.
- The error of "writeFile" is not caught.
- The metadata could contain data other than title and in case it cant be read: writing it seems like it could be harmful.
In the master branch, the dat's metadata is used to copy from and then a write happens on the harddisk. Which means that read errors can not occur and the error on write is also handled:
This seems to be the better way to go about it.
|
Completely agree on this. However, I'm not sure on how I can get the metadata properly, I've attempted a better fix in my last commit. |
| @@ -1,7 +1,7 @@ | |||
| 'use strict' | |||
|
|
|||
| const defaultState = { | |||
| dats: [], | |||
There was a problem hiding this comment.
I don't think this can simply be changed into an object. dats is used as an array to show the list.
There was a problem hiding this comment.
it's used as an object in the reducer (indexed by key if I'm not mistaken). This looked like a typo to me but I'll investigate further!
app/actions/index.js
Outdated
| metadata = await readFile(filePath) | ||
| metadata = JSON.parse(blob) | ||
| } catch (_) { | ||
| metadata = getState().dats[key].metadata |
There was a problem hiding this comment.
I think its ambiguous to use either the file or the metadata, shouldn't this be just the metadata?
There was a problem hiding this comment.
Also: Theoretically the dat doesn't need to exist. Would be good to have an error here if it doesnt
There was a problem hiding this comment.
mhh, I though so as well at first but this action is only used when changing a dat's title in the view so the assumption seemed alright!
|
|
|
ping @martinheidegger I've updated the PR |
|
Hello @soyuka - Sorry for the delay, was stuck in other things. |
Oh no problem wasn't sure you saw my update! Cheers! |
#458 (comment)
Not sure if we should ignore completely the title update if the
dat.jsonfile doesn't exist.