Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: panic for programmer error, more diagnostics, remove duplicated map lookup #126

Merged
merged 5 commits into from
Sep 20, 2023

Conversation

KSXGitHub
Copy link
Contributor

@zkochan Can you test this with the package.json you used for #123 ?

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.13% ⚠️

Comparison is base (94f2848) 85.68% compared to head (a6cade2) 85.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
- Coverage   85.68%   85.56%   -0.13%     
==========================================
  Files          24       24              
  Lines        1411     1413       +2     
==========================================
  Hits         1209     1209              
- Misses        202      204       +2     
Files Changed Coverage Δ
crates/tarball/src/lib.rs 88.80% <0.00%> (-1.45%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.14      8.8±7.51ms   491.5 KB/sec    1.00      7.7±1.97ms   560.6 KB/sec

@zkochan
Copy link
Member

zkochan commented Sep 20, 2023

Which package.json?
This change won't fix the circular dependency issue that I detected while doing #123

@KSXGitHub
Copy link
Contributor Author

KSXGitHub commented Sep 20, 2023

@zkochan I was assuming you tested #123 with a package.json.

Of course, I already tested it with my small package.json and it ran just fine. But the package.json at #116 (comment) meets a "deadlock" (same as #123). I assume that it contains circular dependencies?

Speaking of #116 (comment), if I try it with low semaphore, it will run forever, if I try it with the old codebase, it will panic with too many open files error, and if I try it with #123 and later, it will hangs without logging.

@zkochan
Copy link
Member

zkochan commented Sep 20, 2023

Of course, I already tested it with my small package.json and it ran just fine. But the package.json at #116 (comment) meets a "deadlock" (same as #123). I assume that it contains circular dependencies?

Yes, it contains circular dependencies, so I can't use it to test your changes. I tested your changes on a package.json without circular dependencies.

Speaking of #116 (comment), if I try it with low semaphore, it will run forever, if I try it with the old codebase, it will panic with too many open files error, and if I try it with #123 and later, it will hangs without logging.

All the issues are most likely related to circular dependencies.

@zkochan zkochan merged commit 59a553e into main Sep 20, 2023
@zkochan zkochan deleted the remove-custom-error-type branch September 20, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants