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

Pylint alerts corrections as part of an intervention experiment #401

Open
evidencebp opened this issue Nov 10, 2024 · 3 comments
Open

Pylint alerts corrections as part of an intervention experiment #401

evidencebp opened this issue Nov 10, 2024 · 3 comments

Comments

@evidencebp
Copy link

I'd like to conduct a software engineering experiment regarding the benefit of Pylint alerts removal.
The experiment is described here.
In the experiments, Pylint is used with some specific alerts, files are selected for intervention and control.
After the interventions are done, one can wait and examine the results.

I'm asking for your approval for conducting an intervention in your repository.

See examples of interventions in stanford-oval/storm, gabfl/vault, and coreruleset/coreruleset.

You can see the planed interventions

May I do the interventions?

@FichteFoll
Copy link
Member

FichteFoll commented Nov 11, 2024

Thanks for offering to improve the code of this project. Yes, please go ahead with addressing the pylint notes.

There are no tests, but flake8 is used as a linter (so basically pycodestyle with extras), so it would be advisable to have that run locally. I also just saw that the GitHub Action to run that is actually entirely broken despite showing as passing, so I'll quickly take a look at that. That said, I cannot guarantee that the code base passes the linting check. (Edit: just ran it locally and it does.) I just didn't allow XHR to the host where the step logs were stored and that was rendered as a plain "Error". 🤦

Since I already took a brief look at the planned interventions, you can skip resolving the wildcard import of main.py because that is intentional and related to how the Sublime Text plugin system works. I imagine you'll understand why when you see it. (I also find it curious that it doesn't consider all these wildcard imports.)

@evidencebp
Copy link
Author

Thank you!

In principle, we divide the files into intervention and control groups so we will be able to compare the impact of the interventions.
However, I don't see this file in either group.
It is also from few years ago so it already existed when I run the analysis.
I'll try to rerun and understand that.

@evidencebp
Copy link
Author

I created a PR with the current fixes yet it is still WIP.
@FichteFoll, I'd like to consult you regarding a few points.

In plugins\lib\fileconv\dumpers.py and plugins\lib\fileconv\loaders.py there are unnecessary pass alerts.
The passes are in empty functions with a "To be implemented".
Was the implementation forgotten?
Should the functions raise NotImplementedError?

In plugins\color_scheme_dev.py and plugins\settings\known_settings.py there are too-broad exception alerts due to catching Exception.
Catching Exception might hide unexpected exceptions, now or in future code, and therefore it is recommended to catch specific exceptions.
However, I could not figure out what are the proper exceptions there.Can you guide me?

Last, the main.py file ends with the wildcard import:
from .plugins import *  # noqa
The noqa comment indicates that this is deliberate and OK.
However, I could not understand what is the goal of import in the last line (I did not see main.py being imported).
Out of curiosity, what is the goal of this line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants