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

LGTM warnings #24

Closed
mrdoob opened this issue Apr 21, 2021 · 3 comments
Closed

LGTM warnings #24

mrdoob opened this issue Apr 21, 2021 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@mrdoob
Copy link

mrdoob commented Apr 21, 2021

We use lgtm in three.js and we've noticed web-ifc-api.js has a few warnings:
https://lgtm.com/projects/g/mrdoob/three.js/snapshot/0d37b6de513cc0e33ff7184594bce9b821509152/files/examples/jsm/loaders/ifc/web-ifc-api.js?sort=name&dir=ASC&mode=heatmap

@agviegas agviegas added the bug Something isn't working label Apr 21, 2021
@agviegas agviegas self-assigned this Apr 21, 2021
@agviegas
Copy link
Collaborator

That code is automatically generated by emscripten. It looks like they're just unused variables and redundant boolean evaluations. We'll clean it up programatically to get rid of those warnings!

@agviegas
Copy link
Collaborator

Hey, I've tried to rectify the issues but it doesn't work. Apparently, even though there are unused variables and dubious boolean expressions, WASM needs this code the way Emscripten generates it. I'm not happy with this, but I guess that fiddling with this auto-generated code is not an option.

image

I might be wrong, but I assume that the reason for using a code reviewer is to ensure the quality and maintainability of the repository, avoiding flaws and smells; in the case of this code, it is an output automatically generated by Emscripten (i.e. there is no maintenance and no chance of it being wrong), so maybe it shouldn't be evaluated as if it were code written by us.

I've checked the lgtm documentation and it looks like there are mechanisms to ignore specific files. Do you think this solution would work for Three for this specific file? Sorry for the inconvenience!

@mrdoob
Copy link
Author

mrdoob commented May 6, 2021

I think using LGTM's alert suppression comments should work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants