search

sclorg / rpm-list-builder

RPM List Builder helps you to build a list of defined RPM packages including Software Collection from the recipe file
GNU General Public License v2.0
4 stars 8 forks source link

Fix for pylint unused-argument, unused-variable warnings. #95

Closed junaruga closed 7 years ago

junaruga commented 7 years ago

I ran pylint to know Python better practices in my repository (https://github.com/junaruga/rpm-list-builder/tree/feature/add-pylint) as a trial.

And I fixed below unused-argument and unused-variable warnings. How do you think?

rpmlb/builder/custom.py:29: [W0613(unused-argument), CustomBuilder.before] Unused argument 'kwargs'
rpmlb/builder/mock.py:29: [W0613(unused-argument), MockBuilder.before] Unused argument 'kwargs'
rpmlb/builder/base.py:31: [W0613(unused-argument), BaseBuilder.__init__] Unused argument 'work'
rpmlb/builder/base.py:258: [W0613(unused-argument), BaseBuilder.prepare_extra_steps] Unused argument 'package_metadata'
rpmlb/builder/koji.py:21: [W0613(unused-argument), KojiBuilder.__init__] Unused argument 'extra_options'
rpmlb/downloader/base.py:48: [W0612(unused-variable), BaseDownloader.run] Unused variable 'num_name'
hroncok commented 7 years ago

I don't think it adds readability much. Actually I think it makes the code less readable.

khardix commented 7 years ago

I don't think it adds readability much. Actually I think it makes the code less readable.

Agreed. If we really want this kind of checking, we need a way to mark an argument as "deliberately unused" (i. e. part of the API callback). AFAIK pylint does not provide something like that out of the box, so we would need to come with our own documented convention.

junaruga commented 7 years ago

I agree with you, guys.

There are several cases for the warnings in current source code.

Which case is better to use underscore variable?

I want to investigate those cases more when I have a time. For example, what is a better practice to use a type hints and a kwargs together? (Case 3) So, I want to keep this ticket right now.