Log digest file name when updating trust #12812
Conversation
joyceerhl
requested review from
IanMatthewHuff,
DonJayamanne,
greazer,
rchiodo and
DavidKutu
| @@ -27,6 +27,7 @@ export class DigestStorage implements IDigestStorage { | |||
| const fileLocation = await this.getFileLocation(uri); | |||
| // Since the signature is a hex digest, the character 'z' is being used to delimit the start and end of a single digest | |||
| await this.fs.appendFile(fileLocation, `z${signature}z\n`); | |||
| traceInfo(`Wrote trust for ${uri.toString()} to ${fileLocation}`); | |||
There was a problem hiding this comment.
I'd add a property to track whether we've logged this or not, else we could end up with a lot of unnecessary messages being logged, when we only need the first such log entry.
e.g.
if not loggedd earlier:
traceInfo...
There was a problem hiding this comment.
Then we'd need to remember which files we've seen before, since multiple notebook files could be edited simultaneously and we'd want to log each ipynb's corresponding digest file location at least once. Does your review still apply?
There was a problem hiding this comment.
I think that we would have to do this logging check per file then. Don is right in that this would be so noisy with autosave on that it might not be a useful message if it logs every save.
There was a problem hiding this comment.
I think I would just debounce it. Logging every save isn't that noisy. It's once a second.
There was a problem hiding this comment.
Debouncing won't work as its possible we save a new file and that can get swallowed.
There was a problem hiding this comment.
Debounce will not work, as you loose updates from other files.
There was a problem hiding this comment.
Personally i don't see the point of logging every update, the assumption is that something will go wrong and we will end up looking at the log and the file to look for a specific hash.
I find that to be an unlikely scenario.
Nope, re-read richs comments, it about tracking the last update to the file.
There was a problem hiding this comment.
Not sure we need that either, anyways.
There was a problem hiding this comment.
@greazer suggested, and I agree, that we should log this info at least once for users. It seems plausible that something might go wrong and we won't have a way of recovering that info (short of them giving us the URI to their file and us hashing it to figure out which digest file is relevant).
This info dominates the log, so we shouldn't log on each save either. And as Don mentioned, debouncing won't work. Therefore, anyone have any strong objections to me implementing this? #12812 (comment)
|
Kudos, SonarCloud Quality Gate passed!
|
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).