ci: improve NPM download parameters #379
Conversation
|
/lgtm review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🦉 lgtm Review
Score: Needs Work 🔧
🔍 Summary
This PR does a good job of improving the reliability of the package download script by adjusting request parameters and increasing retries. The refactoring of the Ecosystem definitions into data class instances is also a great change that improves code clarity.
More information
- Id:
e894c90cd32342b8958df1ab207ad848 - Model:
gemini-2.5-pro - Created at:
2025-10-30T14:21:00.277457+00:00
Usage summary
- Request count:
2 - Request tokens:
216,844 - Response tokens:
10,990 - Total tokens:
227,834
See the 📚 lgtm-ai repository for more information about lgtm.
| json.dump(data, fp) | ||
|
|
||
| logger.info("Saved %d packages to `%s` file.", len(set(all_packages)), fpath) | ||
| logger.info("Saved %d packages to `%s` file.", len(all_packages), fpath) |
There was a problem hiding this comment.
🦉 ✨ Quality
Severity: LOW 🔵
logger.info("Saved %d packages to `%s` file.", len(all_packages), fpath)The log message now reports the total number of packages fetched, which might include duplicates if the API returns them across different pages. The previous implementation used len(set(all_packages)) which correctly reports the number of unique packages saved. Reporting the unique count is more informative.
With the current bug in pagination, this change actually hides the issue by reporting a large number of packages when in fact only a small set of unique packages are being fetched repeatedly. Once the pagination is fixed, it would still be better to report on the unique set.
| logger.info("Saved %d packages to `%s` file.", len(all_packages), fpath) | |
| logger.info("Saved %d packages to `%s` file.", len(set(all_packages)), fpath) |
More information about this comment
- File:
dependencies/scripts/download_packages.py - Line:
121 - Relative line:
126 - With suggestion:
Yes - Suggestion ready for replacement:
Yes
1 participant
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 timeout.