-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Run deadcode on edx-platform #32603
Comments
Hi, @dianakhuang. I've been trying to contribute to open Edx for a while now and this seems like something comfortable enough for me to get started with. Should I work on this issue? And I'm quite new to open source so if I breach any common practices or make any mistakes, please correct me. I'm here to learn and add value to this repo. |
@mehedikhan72 we would welcome the help! Feel free to come visit us at the deprecation working group as well to talk over your findings and : https://discuss.openedx.org/t/announcing-the-deprecation-working-group/6173 I sketched out this ticket during a meeting, so feel free to ask for more information/details. This version of the ticket is very bare bones. |
@dianakhuang Thank you. I appreciate that. I ran the following command on the edx-platform. |
@dianakhuang the results are here. I've attached the txt file with this comment. Please check it out and let me know if it's alright. |
Hi @mehedikhan72 ! We discussed this during the DEPR working group meeting, and unfortunately, the results in that document are a little too noisy. It's still including tests, for example, and Django views that are not clearly referenced by other files. If you still have time to work on this, could you look into how to configure |
Hi @dianakhuang! I think I can configure it such that tests are ignored. However, how do I know which django views are not clearly referenced by other files? If you could clairfy me on that, I could run the deadcode again. Thanks! |
I am not sure if there's a good way to find out if specific views get referenced elsewhere, but for example:
I believe these endpoints can be called from the frontend, it's just that deadcode can't see those code paths, since they're coming from the frontend. One resource that might be helpful is this blog post about using a different, but similar tool with Django. |
Hi @dianakhuang . I have the updated results on edx-platform, both using vulture and deadcode. Tests are ignored this time. What should I do next regarding this? Do you want me to start working on removing unused variables? Since functions might be connected with API endpoints, I think removing unused variables might be a good place to start with. |
Thanks @mehedikhan72 ! We'll take a look at these new results and discuss during the DEPR working group meeting today. |
There's a script at https://github.com/openedx/edx-platform/blob/master/scripts/vulture/find-dead-code.sh for fine-tuning the vulture settings a bit to eliminate false positives, do you want to try that and see how it compares? I don't think we've run it in a while, so it might need some updates. |
@dianakhuang alright, please keep me updated and let me know what I can do next. Hi @jmbowman , I'll give it a look and let you know what I find. |
Hi @jmbowman , this script produces only a few dead code instances since it excludes dead code instances with minimum confidence. Previously the script was checking on cms, lms, common and openedx folders only. I checked on conf, docs, pavelib and xmodule as well. Here are the results: From what I've read so far, removing codes where vulture gives a 90% and 100% confidence is a safe option considering, it shows a 90% confidence for unused imports and a 100% confidence for codes that cannot run logically. Should I make a PR getting rid of these? |
This is great @mehedikhan72 ! Much less noisy than the previous ones. I do think there's a few false positives floating still around in there (the |
@dianakhuang did you see the latest one where deadcodes with a high confidence is shown only? It ignores less confident results. And how did you know that the |
@mehedikhan72 Sorry it's taken so long to get back to you. I've been a bit busy the last couple of weeks. Talked it over with the team, and we're thinking it would make sense for a PR of the 100% confidence suggestions at first if you still have the time and interest in doing so. I only knew that the |
@dianakhuang no worries. I'm working on it. |
Run https://github.com/albertas/deadcode on edx-platform.
Acceptance Criteria
The text was updated successfully, but these errors were encountered: