Pylint alerts corrections as part of an intervention experiment

[evidencebp]

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]

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]

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]

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?

[FichteFoll]

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?

No, the implementaiton was not forgotten. These files/classes use polymorphism and the methods should be implemented in sub-classes. They should raise NotImplementedError or be decorated with abc.abstractmethod.

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.

The exceptions in plugins/color_scheme_dev.py:124 and plugins/settings/known_settings.py:200 (by the way, mentioning the line number helps finding it quicker) are logged, so that is a fine use case of a generic catch in my book.

Regardless, from what I know, sublime.decode_value generally raises ValueError when it cannot parse the provided string and sublime.read_resource probably raises some sort of IOError, but I'm not certain of that as it's not trivial to replicate and not documented.

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?

Sublime Text loads all top-level modules in a package (here: repository root) and inspects the module globals. Subclasses of the sublime_plugin classes and two functions plugin_loaded and plugin_unloaded are collected and inserted into the plugin lifecycle. The star import is basically just a re-export of plugins/__init__.py, which then serves as the main entry point for the plugins.

[evidencebp]

Thank you for your detailed and useful feedback!

I will adapt the PR accordingly. As you suggested in the PR, I'll revert the too problematic commits. Thank you for doing them yourself! By the way, we plan to add people that do 50 interventions as authors so you are already on the way...

Your insights are great and helped me. The paper itself is a bit far but this is the content that we look for ;-)