Move all URLs to a separate file & improve getbin method #762
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #762 +/- ##
==========================================
+ Coverage 82.51% 82.55% +0.03%
==========================================
Files 141 142 +1
Lines 11527 11527
Branches 2133 2133
==========================================
+ Hits 9512 9516 +4
+ Misses 1682 1679 -3
+ Partials 333 332 -1 ☔ View full report in Codecov by Sentry. |
|
The code coverage test seems to be failing, and I'm not sure of the root cause. Does it mean that the existing tests are not compatible with the changes made in the MR and require updating the test(s)? How do I make sure that the code coverage tests pass? Is there any way I can check the code coverage results locally before raising a PR? Running |
src/utilities.ts
Outdated
| @@ -409,6 +409,11 @@ export function download(query, viewer, options, callback?) { | |||
| var promise = null; | |||
| var m = viewer.addModel(); | |||
|
|
|||
| if(isRequestProcessing){ | |||
There was a problem hiding this comment.
I do not see why you wouldn't want to allow multi-threaded downloads. Serializing them is not a feature.
There was a problem hiding this comment.
Ummm...I'm not sure of the use case for downloading the same data multiple times, and users might unintentionally end up performing the same action multiple times due to a lack of visual indication and hence I feel we should throttle the action. Kindly let me know if I'm missing something here.
There was a problem hiding this comment.
Why do you assume it is the same data? getbin is a general utility. It is not acceptable for someone to try to fetch two different files at the same time and get an error on the second one.
There was a problem hiding this comment.
getbin is a general utility. It is not acceptable for someone to try to fetch two different files at the same time and get an error on the second one.
I 100% agree with this. However, the throttling functionality was added only to the download method and not getbin so fetching multiple files from getbin shouldn't block multiple downloads at once. Anyway, I further double-checked to see if the download method is being used elsewhere and I could see that it is used in example.html of tests and in several html files of tests/webpages. And, it turns out that in webpages/splitviewer.html the download method is invoked in a loop and throttling would cause a bottleneck here. I had overseen this earlier and hence I was not aware. I shall remove the throttling changes and push a commit. Thank you for bringing this to my attention :)
There was a problem hiding this comment.
More to the point, it is part of the public API so anyone can use it.
There was a problem hiding this comment.
Right. Going ahead, I'll keep a broader view when proposing changes.
💫 Changelog
⭐ In https://3dmol.org/doc/index.html there's a viewer embedded with a download option that can be clicked continuously and an equivalent number of requests will be made, this is not efficient as it'll duplicate multiple requests. A network call on the same action should be allowed only after the previous call fails/succeeds. Throttling has been added so that no duplicate and unnecessary requests are sent.
⭐ All the URIs have been moved to a new file
URLs.ts. If any changes or additions are supposed to be made, it can be updated in one place instead of changing at multiple places which could also be prone to typos.⭐ Code duplicate in
getBinfunction is removed.