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

refactor: Pylint alerts corrections as part of an intervention experiment 298 #1022

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

evidencebp
Copy link

@evidencebp evidencebp commented Dec 1, 2024

Makes the interventions describe in intervention issue.
The experiment is described here.

Each intervention was done in a dedicated commit with a message explaining it.

Removed unnecessary parenthesis
Method do_setup had 15 branches and function get_versions had 13 branches while Pylint recommends to have no more than 12.
The function build checks the value of "pkg_dir is None" in a if statement and assigns it to the variable use_host_conda_bld (line 469).I assigned the expression directly to the variable, removing the unneeded if and simplifying the code.
The function build had 66 statements and the function  build_recipes   had 86, while Pylint recommends having no more than 50.

Functions were structured so I extracted methods to handle specific logics, simplifying the code.
In the class Recipe the method replace  had 13 branches and the static method _rewrite_selector_block  had 14, while Pylint recommends having no more than 12.

I extracted small methods to reduce the complexity.
 The function "check" had 14 branches , while Pylint recommends having no more than 12. I extracted a method to handle the metas.
 The function  run had 13 branches and, while Pylint recommends having no more than 12.  I extracted _handle_process to reduce the complexity.
 The method apply of the class CreatePullRequest  had 14 branches and, while Pylint recommends having no more than 12.
I extracted _handle_open_PRs to reduce the complexity.
@evidencebp evidencebp changed the title Pylint alerts corrections as part of an intervention experiment 298 refactor: Pylint alerts corrections as part of an intervention experiment 298 Dec 1, 2024
@jmarshall
Copy link
Member

  • Please use slashes rather than backslashes to delimit paths in your commit messages.

  • versioneer.py is external code copied into bioconda-utils. It is a mistake to alter this upstream code locally.

  • IMHO these too-many-branches and too-many-statements changes in fact make this code less understandable, by moving code further away from the code it is related to and by introducing badly-named functions that implement meaningless abstractions. So my opinion would be not to apply these particular changes.

@evidencebp
Copy link
Author

@jmarshall , I'll exclude versioneer.py

As for the too-many interventions, in this PR the code is not very-much-too-many.
Your claim about locality is reasonable and that is what makes these cases interesting for the experiment.
If you find the intervention good enough, merging them will allow us to investigate the change in tendency to bugs, ease of modifications, etc.
Are you OK with that?

@jmarshall
Copy link
Member

It's not my decision. If it was, I would have just closed WONTFIX your initial issue. Your experiment may be interesting to you, but it's mostly a waste of time for the projects you're experimenting on.

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

Successfully merging this pull request may close these issues.

2 participants