Fix lgtm.com alerts: equality tests #11348
Conversation
details: https://lgtm.com/projects/g/mrdoob/three.js/snapshot/dist-10110011-1494681683902/files/examples/js/loaders/MMDLoader.js#V430 https://lgtm.com/projects/g/mrdoob/three.js/snapshot/dist-10110011-1494681683902/files/examples/js/loaders/MMDLoader.js#V1589 https://lgtm.com/projects/g/mrdoob/three.js/snapshot/dist-10110011-1494681683902/files/examples/js/loaders/MMDLoader.js#V1631
|
@lgtmhq is cool! Not sure about your proposed fixes though... I'll have to take a closer look. |
| @@ -427,7 +427,7 @@ THREE.MMDLoader.prototype.pourVmdIntoCamera = function ( camera, vmd, name ) { | |||
|
|
|||
| var clip = new THREE.AnimationClip( name === undefined ? THREE.Math.generateUUID() : name, -1, tracks ); | |||
|
|
|||
| if ( clip !== null ) { | |||
There was a problem hiding this comment.
@takahirox can clip be null here?
There was a problem hiding this comment.
Maybe no.... I don't remember why I added this null check logic....
Seems like I should remove.
There was a problem hiding this comment.
I've checked the history.
I used THREE.AnimationClip.parseAnimation() which can return null before.
Seems like I've forgotten to remove null check logic when I replaced it with new THREE.AnimationClip().
There was a problem hiding this comment.
@takahirox FYI lgtm.com reports a few more warnings in MMDLoader:
https://lgtm.com/projects/g/mrdoob/three.js/snapshot/bf66a7672bac97558f48a8bec4cfc4bcf0382da4/files/examples/js/loaders/MMDLoader.js
There was a problem hiding this comment.
OK, I'll fix them later!
|
@mrdoob @takahirox: thanks for your careful review, let me know if there's anything else I can do to help. Would love to hear your feedback on the automated lgtm.com code review for GitHub pull requests. You can enable it here: https://lgtm.com/projects/g/mrdoob/three.js/ci/. That way you get notified of alerts before they get merged into master :) |
|
@sjvs |
|
I'll clean all this. |
|
Done. Thanks! |
|
@sjvs I spent the whole day yesterday fixing 200 alerts in https://lgtm.com/projects/g/mrdoob/three.js/ The page reported 810 alerts yesterday, today it reports 937... 🤔 |
|
@mrdoob: there was a slight glitch earlier today; things are on their way back to normal. We're in the process of re-analysing the most recent revision of three.js, results should appear soon on https://lgtm.com/projects/g/mrdoob/three.js/alerts/ We've just updated the queries that generate the alerts, which should result in fewer false positives and more true positives. Therefore, the number of alerts may not quite be the same as yesterday's minus 200. Let me know if you see anything unexpected! |
|
@sjvs thank you! lovely service by the way! |
|
No problem, @mrdoob - happy to help. We're very keen to hear feedback from early adopters like yourself, especially regarding the automated reviews of pull requests which we recently introduced. Here's an example of how the guys at NASA's JPL are using it for PRs on one of their open-source projects: Open-MBEE/exec-cameo-mdk#105. The automated review will catch any alerts so you can fix them before merging a PR into master. It'd be great if you could give that a go for Three.js, and let me know your feedback so we can improve :)! You should be able to enable it here: https://lgtm.com/projects/g/mrdoob/three.js/ci/ |
|
Yep, just added it. (First time I add anything tot this repo btw 😊) |
|
Great - thanks! Now all we need is a new pull request to see it in action! You're one of the first users, so do let me know if there's anything you'd like to see improved. That's the perk of being an early adopter 😉 |
|
Will do! 👍 |
This PR fixes a number of alerts reported by lgtm.com (@lgtmhq). There is a good number of other alerts well worth looking at here: https://lgtm.com/projects/g/mrdoob/three.js/alerts/
You can set up automated pull request reviews on lgtm.com as well!