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 #1536

Closed
evidencebp opened this issue Nov 9, 2024 · 8 comments
Closed

Pylint alerts corrections as part of an intervention experiment #1536

evidencebp opened this issue Nov 9, 2024 · 8 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

@aaugustin , may I do the interventions?

@aaugustin
Copy link
Member

I understand that the starting point would be https://github.com/evidencebp/pylint-intervention/blob/main/interventions/candidates/aaugustin_websockets_interventions_October_05_2024.csv.

The first line is too-many-branches. I'm not interested in refactoring code that works well and that I'm happy with for the sake of making a linter happy. So, the answer is "no".

@aaugustin
Copy link
Member

I looked at a few other lines and many of them aren't actual problems — e.g. that wildcard import is fine.

@aaugustin
Copy link
Member

I'll keep this issue open in case I want to run pylint once (blast from 15 years ago!) and see if it catches some interesting stuff.

The hard part isn't running pylint or changing the code; it's making the judgment call of whether the code is OK as it stands or not.

It will be way faster to do it myself directly than by reviewing a pull request.

@aaugustin
Copy link
Member

What settings are you using? With zero configuration I'm getting:

$ pylint src tests | wc -l
    4378

which is way more than I'm willing to read (also why this project uses a helpful linter rather than pylint).

@aaugustin
Copy link
Member

I looked at the first ten lines of your spreadsheet and none of them has value. See the list below for details.

That's a data point for your study in the value of removing pylint warnings :-)

As far as websockets is concerned value is zero.

I recommend that you look at ruff for a fast, modern, pragmatic, Python linter.


src/websockets/asyncio/server.py:111:4: R0912: Too many branches (14/12) (too-many-branches)

code is OK

src/websockets/server.py:174:15: W0718: Catching too general exception Exception (broad-exception-caught)
src/websockets/server.py:554:19: W0718: Catching too general exception Exception (broad-exception-caught)

pylint is wrong

src/websockets/sync/connection.py:317:4: R0915: Too many statements (69/50) (too-many-statements)

code is OK

src/websockets/client.py:320:19: W0718: Catching too general exception Exception (broad-exception-caught)

pylint is wrong

src/websockets/sync/server.py:319:0: R0915: Too many statements (65/50) (too-many-statements)

pylint doesn't realize there's a function defined inside this function

src/websockets/sync/client.py:123:0: R0915: Too many statements (56/50) (too-many-statements)

code is OK

src/websockets/http11.py:204:4: R0912: Too many branches (15/12) (too-many-branches)

code is acceptable — splitting reading the response body to a separate function and calling that function wouldn't be an clear improvement

src/websockets/main.py:132:11: W0718: Catching too general exception Exception (broad-exception-caught)

pylint is wrong

src/websockets/legacy/protocol.py:1:0: C0302: Too many lines in module (1641/1000) (too-many-lines)

there's a reason why I rewrote this code... but I'm going to keep the original for 5 years after deprecation — meaning, counting from today — and there's no way I'm going to take the risk of refactoring it

experiments/broadcast/clients.py:37:11: W0718: Catching too general exception Exception (broad-exception-caught)

this is a one-off experiment; no matter whether the warning is justified, I'm not investing into polishing exception handling

@evidencebp
Copy link
Author

Thank you very much for your detailed feedback, @aaugustin !

I agree that in your case most alerts are not dramatic.
Part of the experiment is to identify the alerts whos benefit is limited.

As for src/websockets/legacy/protocol.py:1:0: C0302: Too many lines in module (1641/1000) (too-many-lines)
Refactoring it does seems like hell.
Do you think that had you invested in that, it would have been beneficial?

@aaugustin
Copy link
Member

I decided to refactor because the module was too complex. The module was too long because it was too complex. So there's a relationship between these two facts.

But "it's too long" doesn't give a clue about how to refactor it. I decided to refactor when I learnt about the Sans-I/O design pattern.

It too me 6 years and 2 months from "having the idea to refactor" (cf. #466) to "deprecating the legacy implementation" (yesterday). I'd never embark into 6+ years of work because a module is 1641 lines long. It's nowhere near a large enough problem to warrant 6+ years of work.

@evidencebp
Copy link
Author

Oh, sure.
Congrutiolions for finishing this 6 years effort!

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