xbmc / addon-check

Automatic checks for new repository submissions
GNU General Public License v3.0
52 stars 42 forks source link

Switched from pycodestyle to pylint #174

Closed LordGameleo closed 5 years ago

LordGameleo commented 5 years ago

173

Note: It will not pass the build. We will have to correct it. I have created new issue for it. #175

mzfr commented 5 years ago

I think we should have a .pylintrc too. So we can configure pylint. Because pylint have lots of options of suggestions and stuff which might not be needed.

LordGameleo commented 5 years ago

What I propose is:

@Razzeee @Rechi @mzfr What do you think about it?

LordGameleo commented 5 years ago

I disabled these checks to get a 10 on 10 score for now.

        broad-except,
        too-few-public-methods,
        parse-error,
        invalid-name,
        attribute-defined-outside-init,
        pointless-statement,
        useless-object-inheritance,
        no-else-return,
        no-name-in-module,
        len-as-condition,
        missing-docstring,
        bad-whitespace,
        duplicate-code,
        unused-import,
        unused-variable,
        useless-else-on-loop,
        bad-continuation,
        import-error,
        too-many-arguments,
        logging-not-lazy,
        undefined-variable,
        syntax-error,
        unidiomatic-typecheck,
        too-many-nested-blocks,
        inconsistent-return-statements,
        arguments-differ,
        consider-using-in,
        deprecated-method,
        redefined-outer-name,
LordGameleo commented 5 years ago

175

This issue will be broken into several parts for example:

razzeee commented 5 years ago

Sounds sensible.

My preferred setup would be to rely on the standard configuration and not configuring it at all. But getting there will take time. Am I correct that we should be able to just delete the config file then and it will just use pylint defaults?

mzfr commented 5 years ago

@Razzeee Running pylint with default config is possible and actually isn't a big task to achieve because there aren't many errors left, the most amounts of errors are about snake_case vs the camelCase so we need to decide that and then make sure everyone use that. Other than that just the docstrings and Do not uselen(SEQUENCE)to determine if a sequence is empty errors are there. Which I think won't be a problem to fix.

That being said I still think that running pylint on complete default isn't a good idea(at least in my opinion) because pylint gives quite a few suggestions of itself and sometime can have False-positives.

LordGameleo commented 5 years ago

@Razzeee it is possible and I am also suggesting that. As we progress with individual issues we will remove them one by one from disabled list. And once all these issues are solved we will be left with the default configuration. Its like we will be doing it in progressive way.

LordGameleo commented 5 years ago

I disabled these checks to get a 10 on 10 score for now.

        broad-except,
        too-few-public-methods,
        parse-error,
        invalid-name,
        attribute-defined-outside-init,
        pointless-statement,
        useless-object-inheritance,
        no-else-return,
        no-name-in-module,
        len-as-condition,
        missing-docstring,
        bad-whitespace,
        duplicate-code,
        unused-import,
        unused-variable,
        useless-else-on-loop,
        bad-continuation,
        import-error,
        too-many-arguments,
        logging-not-lazy,
        undefined-variable,
        syntax-error,
        unidiomatic-typecheck,
        too-many-nested-blocks,
        inconsistent-return-statements,
        arguments-differ,
        consider-using-in,
        deprecated-method,
        redefined-outer-name,

@Rechi I just disabled these checks which was causing the build to fail.... I used the feature of pylint which creates pylintrc file. Everything except what I mentioned above is default. I used standard method of pylint to create it. We can remove all other things if we want to.

Rechi commented 5 years ago

This is now a more reasonable list, then you have in the current .pylintrc file. The reason why parse-error, pointless-statement, syntax-error and undefined-variable have to be ignored are not actual code errors, but linting LICENSE, MANIFEST.in, PUBLISH.md, requirements.txt and script.test. Also I don't have to exclude no-name-in-module, bad-whitespace and import-error.

LordGameleo commented 5 years ago

@Rechi for confirming.... Do you want me to make my own customized version of pylintrc file which includes only these errors which causes the build to fail. And yes, we are ignoring these errors just so we can merge this branch successfully without failing travis build. We will solve these errors in new issues and when they are those issues are solved we will remove those disabled errors from pylintrc( specific to these disabled ones ) and we will keep ignoring those errors which we don't have to ignore.

LordGameleo commented 5 years ago

@Razzeee @mzfr @Rechi Is there anything to change here?

LordGameleo commented 5 years ago

This is now a more reasonable list, then you have in the current .pylintrc file. The reason why parse-error, pointless-statement, syntax-error and undefined-variable have to be ignored are not actual code errors, but linting LICENSE, MANIFEST.in, PUBLISH.md, requirements.txt and script.test. Also I don't have to exclude no-name-in-module, bad-whitespace and import-error.

@Rechi I am sorry I guess I couldn't get what you exactly said here. I got it now, I will fix these. Sorry for making you do the extra work

LordGameleo commented 5 years ago

@Rechi Check this once and tell me if you see anything now.

Rechi commented 5 years ago

pip install pylint is missing in .travis.yml.

LordGameleo commented 5 years ago

@Razzeee @Rechi Please Review :)