-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Go over findings from static analyzers #11965
Comments
Recently LGTM has actually been helpful in flagging a couple of valid logical mistakes in new code, see e.g. PRs #13694 and #13712, so I'm wondering if we should actually reconsider the decision to not use its PR integration? Looking e.g. at https://lgtm.com/help/lgtm/github-apps-integration, the setup doesn't appear to be particularly difficult. |
Given that no objections have been raised here, I'll try to get the LGTM PR-integration setup sometime this week (assuming I've got the required permissions). |
Sorry, I forgot to reply. I agree that LGTM can be useful to integrate now that it has given some useful results and the false positives are (largely) silenced. |
As I (somewhat) suspected, enabling the LGTM PR-integration seem to require repository ownership since simple commit access doesn't appear to be enough. |
Looking e.g. at https://github.com/mozilla/pdf.js/pull/13761/checks or https://github.com/mozilla/pdf.js/pull/13754/checks, it seems that LGTM has now been setup and that it's working :-) |
https://lgtm.com/projects/g/mozilla/pdf.js?mode=list is interesting and might provide PDF.js with some opportunities to improve the code.
The text was updated successfully, but these errors were encountered: