eugene eugene
  • Joined on 2024-07-03
eugene approved sigit/sigit.io#304 2025-01-22 11:52:49 +00:00
Offline flow separation
eugene commented on pull request sigit/sigit.io#304 2025-01-22 11:52:24 +00:00
Offline flow separation

I think it looks good. While the PR description is good/clear, it would be easier to analyse the code if a separate PR was opened for each issue (appreciate that it's not always possible or makes…

eugene commented on pull request sigit/sigit.io#278 2025-01-15 10:22:11 +00:00
issue-274

Ok, thanks.

What is the reason to add break at the end of the last loop? Isn't the loop going to finish naturally because you increment i?

A bit of a nitpick, but I would also suggest…

eugene commented on pull request sigit/sigit.io#278 2025-01-15 10:08:41 +00:00
issue-274

You could have an entity FileServer:

url: string, read: boolean, write: boolean servers would hold an array of FileServer objects.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:34:32 +00:00
issue-274

You could use .map to collect promises in Promise.all to make the code more declarative.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:28:52 +00:00
issue-274

It would be better to wrap the whole body of the function in the try catch block, rather than just catch an error in this one instance.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:27:04 +00:00
issue-274

Should it be configurable as well?

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:22:16 +00:00
issue-274

I think Servers is too ambiguous to use as a type. It should either be FileServer or BlossomServer. FileServer is a better option if we want to allow for a possibility that in the future there will be other compatible standards other than Blossom.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:16:29 +00:00
issue-274

You don't need this if statement. getHash returns a string or null, so you can directly assign hash instead of re-assigning null.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:14:06 +00:00
issue-274

Can you specify why it needs to break out of the loop if it's the last zip url?

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:08:49 +00:00
issue-274

The same question as before - do we need noFiles, and if yes, can you some comment as to why.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 12:02:05 +00:00
issue-274

Does it mean that if a user has 3 blossom servrs, there will be 2 requests?

eugene commented on pull request sigit/sigit.io#278 2025-01-14 11:54:25 +00:00
issue-274

Sounds like a pretty critical issue. Can you please add more information and create an issue?

eugene commented on pull request sigit/sigit.io#278 2025-01-14 10:33:17 +00:00
issue-274

Similar to my point elsewhere in the PR, I don't think we should be storing blossoms in a map. It should be an array instead.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 10:31:12 +00:00
issue-274

I don't think you need to reject the promise in this scenario. You can return a resolved promise with either true (if you received the expected response) or false, and you won't need to do rely on catching the error in the client code to make a business logic decision.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 10:21:17 +00:00
issue-274

You should use either the async await syntax or promise chaining, not both of them at the same time.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 10:14:46 +00:00
issue-274

Should it be added at the end of the array instead? It would make sense, going from the first version to the last version, i.e. 1, 2 .... n

eugene commented on pull request sigit/sigit.io#278 2025-01-14 10:06:35 +00:00
issue-274

Why not keep the existing property, blossomUrls? It looks like the contect is exactly the same, an array of strings.

eugene commented on pull request sigit/sigit.io#278 2025-01-14 10:02:25 +00:00
issue-274

This transformation can be avoided if you change serverMap to be an array.