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 intervention experiment #434

Open
evidencebp opened this issue Oct 8, 2024 · 7 comments
Open

Pylint alerts corrections as part of intervention experiment #434

evidencebp opened this issue Oct 8, 2024 · 7 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 your approval for conducting the interventions in your repositories.The interventions are expected to benefit the project and will submitted in PR for approval.

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

See the planed internetions.

@brechtm, Is it OK if I'll fix the alerts?

@brechtm
Copy link
Owner

brechtm commented Oct 17, 2024

Of course, code improvement are appreciated! However, it's not clear to me how much effort this requires on my part. Since I don't have much free time nowadays, I cannot promise I can respond in a timely manner.

@evidencebp
Copy link
Author

I'll try not to require too much from you.
I'll do the interventions.
In case that I'll have questions, I'll be happy to consult.

After that, you'll ask you to review the PR.
Most interventions are simple and I'll document and justify them so I hope it will not take you too much time.

@evidencebp
Copy link
Author

I fixed the pylint alert and created a PR.
Note that this is still work in progress yet I wanted to consult you.

I notice that the setup for development doe not use requirements file.
Is setting the development environment done using pyenv_setup.py? Can you guide me?

@evidencebp
Copy link
Author

The alert in src\rinoh\hyphenator.py seems to identify a bug and not a refactoring opportunity.all = ("Hyphenator") while it should be a list (e.g. all = ["Hyphenator"]). https://stackoverflow.com/questions/44834/what-does-all-mean-in-python

In src\rinoh\highlight.py and src\rinoh\frontend\sphinx\nodes.py there are alerts on using-constant-test.
In both cases the function "warn" appears in the if statement but not as a call (e.g., warn()).
Later the function is called. What is the intention?
elif warn:
warn('Could not lex literal_block as "%s". '
'Highlighting skipped.' % language)

@evidencebp
Copy link
Author

In src\rinoh\frontend\rst\nodes.py there is an alert on pointless statment.
Are properties being created on the fly and the self.title aimed to check the existence?
class Admonition(DocutilsGroupingNode):
grouped_flowables_class = rt.Admonition

def children_flowables(self, skip_first=0):
try:
self.title
return super().children_flowables(skip_first=1)
except AttributeError:
return super().children_flowables()

@evidencebp
Copy link
Author

In src\rinoh\element.py method build_document of class DocumentElement has an empty implementation of pass.
It is implemented in subclasses. Might be better to enforce it by raising NotImplementedError.

@evidencebp
Copy link
Author

In src\rinoh\stylesheets\matcher.py there is a wildcard import.
Code uses from rinoh.styleds import *
This hides the imported objects and might lead to collisions in the future if objects of the same name are imported with wildcards.  styleds is a collection of imports, aimed to ease related imports.
I copied the used imports from there and deleted the unused ones.  
This adds many imports and does not  benefit from styleds, so this is a downside of fixing this alert.

@brechtm, can you guide me regadding these cases?

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

3 participants
@brechtm @evidencebp and others