-
Notifications
You must be signed in to change notification settings - Fork 5
ci: improve NPM download parameters #380
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
Conversation
f535de8 to
6772bde
Compare
|
/lgtm review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦉 lgtm Review
Score: Nitpicks 🤓
🔍 Summary
This PR does a great job of improving the robustness of the NPM package download script. The changes to increase retries, lower the page size, and increase the timeout are correctly implemented. The refactoring to use instances instead of classes for ecosystem configurations and the use of the @stamina.retry decorator are excellent improvements that make the code cleaner and easier to understand.
I've pointed out a couple of areas for improvement:
- A potential side effect where a shared dictionary is mutated within a loop.
- An opportunity to ensure the stored package list is free of duplicates, which would also make the logging more accurate.
Overall, this is a solid PR that is almost ready for merging.
More information
- Id:
987a661f6c0e43d68f2bed738a1d5fdf - Model:
gemini-2.5-pro - Created at:
2025-10-30T14:29:16.632338+00:00
Usage summary
- Request count:
2 - Request tokens:
216,766 - Response tokens:
11,435 - Total tokens:
228,201
See the 📚 lgtm-ai repository for more information about lgtm.
6772bde to
232c741
Compare
|
/lgtm review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦉 lgtm Review
Score: Needs Work 🔧
🔍 Summary
The pull request does a good job of improving the robustness and readability of the package download script. The move to constants, the refactoring of ecosystem definitions into dataclass instances, and the switch to the @stamina.retry decorator are all excellent changes.
However, there is a critical bug in the loop that fetches paginated packages. The use of set.union() without assignment means that the set of packages is not updated, resulting in an empty or incomplete list of packages for paginated sources like NPM.
Once this bug is fixed and a minor unused import is removed, the PR will be in great shape.
More information
- Id:
1446191262db4220aec6e4f4cd3652f1 - Model:
gemini-2.5-pro - Created at:
2025-10-30T14:40:14.035963+00:00
Usage summary
- Request count:
2 - Request tokens:
219,070 - Response tokens:
8,325 - Total tokens:
227,395
See the 📚 lgtm-ai repository for more information about lgtm.
232c741 to
6fda1ff
Compare
The api from where we retrieve the top NPM packages was some times returning 5xx errors, so I've lowered the number of items per page and increased the number of retries and the timeout.