pylint code checks #9

de-code · 2018-08-16T16:56:17Z

There are some pylint issues with implicit namespace packages:

I'm proposing to just add the __init__.py files for now.

I am planning to create separate PRs for other code checking tools.

de-code · 2018-08-17T17:05:36Z

Dockerfile.venv

@@ -2,6 +2,8 @@ ARG python_base_image_tag
 FROM python:${python_base_image_tag}
 RUN pip install --no-cache-dir --only-binary --upgrade pipenv

+RUN apk --no-cache add linux-headers g++


Need to check whether that can be reduced more

de-code · 2018-08-17T17:06:23Z

project_tests.sh

+
+set -e
+
+pylint content_store/api/*.py


This needs to be more generic (but pylint content_store doesn't work because of it not containing an __init__.py file)

… warning

de-code · 2018-08-20T10:09:03Z

.pylintrc

+
+[BASIC]
+good-names=
+    id


not sure if id is actually a good name, given that it's also a built-in function

de-code · 2018-08-20T10:09:50Z

.pylintrc

@@ -0,0 +1,14 @@
+[MESSAGES CONTROL]
+disable=
+    fixme,


might remove fixme later - we have a TODO in there; probably should not keep TODOs in the code

de-code · 2018-08-20T10:11:14Z

.pylintrc

+[MESSAGES CONTROL]
+disable=
+    fixme,
+    missing-docstring,


better to structure the code in a read-able way and add high-level documentation

de-code · 2018-08-20T10:12:22Z

.pylintrc

+disable=
+    fixme,
+    missing-docstring,
+    too-few-public-methods


better to separate code logically, even if one module ends up having only one public method for now

de-code · 2018-08-20T10:28:55Z

content_store/__init__.py

@@ -0,0 +1 @@
+# for pylint


I'm proposing to just add the __init__.py files rather than messing around with an untidy pylint workaround.

de-code · 2018-08-20T11:03:25Z

content_store/api/api.py


-    # TODO : add migrations
+    # database setup (will use migration in the future)


reworded to not require fixme pylint exception

de-code · 2018-08-20T11:04:45Z

content_store/api/api.py


    # could move to blueprint
    @app.route("/ping")
-    def ping():
+    def _ping():


with the underscore pylint won't complain about the function not being used (seems to be a common pattern)

de-code · 2018-08-20T11:10:40Z

project_tests.sh

@@ -0,0 +1,6 @@
+#!/bin/sh


I'm using the project_tests.sh file also directly (without docker); it's faster than waiting for the docker build and then run the tests within it.

Naming format, want to uniform to project-tests.sh?

Renamed (didn't realise you did adopt the hyphen in the end)

giorgiosironi · 2018-08-20T13:39:00Z

Dockerfile.venv

@@ -2,6 +2,8 @@ ARG python_base_image_tag
 FROM python:${python_base_image_tag}
 RUN pip install --no-cache-dir --only-binary --upgrade pipenv

+RUN apk --no-cache add linux-headers g++


Pylint (or one of its dependencies) requires a C++ compiler for the Python package to be installed? At least it doesn't need to be carried over to the main image

I've seen pylint-dev/pylint#1423 for example

I encountered it in typed_ast (ast27/Parser/acceler.c). I guess it kind of makes sense that dependencies use native code to make bottlenecks faster.

giorgiosironi · 2018-08-20T13:44:12Z

project_tests.sh

+
+set -e
+
+pytest && \


python -m pytest is usually different from pytest as it adds . to PYTHONPATH. In the container this is probably not that useful as we can set that env variable in the Dockerfile; what about on the host?

I did not know that. Where do you see that it's different? For me the script is just calling pytest.main, as seem to be the case when when running it as a module too. So not sure where it is adding it to the path.

Although I did see a difference when calling pytest without the __init__.py in tests then it failing to find content_store, but it seems to work with python -m pyest. Its working with both commands if there is a __init__.py file in tests.

Long story at https://docs.pytest.org/en/latest/pythonpath.html but if it works, good

giorgiosironi

Will try this out after the schema meeting

giorgiosironi · 2018-08-21T08:31:17Z

Pipfile

@@ -5,6 +5,7 @@ name = "pypi"

 [dev-packages]
 pytest = "~=3.7.1"
+pylint = "*"


Specify at least the major version?

Okay, added. Even though it doesn't seem to be common practise for pipenv.

Apparently that is coming out of pipenv:

We recommend updating your Pipfile to specify the "*" version, instead.

The documentation isn't as clear about it apart from assuming that fixed versions come from the requirements.txt.

The lock file deals with the exact version separately, but a major version here (can't see it at the moment?) should be handy as if we run an upgrade we don't want to end up with an hypothetical future Pylint 3 that has introduced backward-incompatible changes like a different format for .pylintrc, etc.

I understand that people importing a requirements.txt will end up with things like 1.2.3.4rc2_0ubuntu1 which is brittle.

I accidentally put it on the wrong dependency (fixed).

I understand reasons for add a major version.

But we should be aware that it does seem to be against the advice of pipenv.

giorgiosironi · 2018-08-21T08:32:54Z

content_store/api/database.py

@@ -1,2 +1,3 @@
 from flask_sqlalchemy import SQLAlchemy
-db = SQLAlchemy()
+
+DB = SQLAlchemy()


correct, module-level constant needs to be uppercase

giorgiosironi · 2018-08-21T08:34:02Z

project-tests.sh

+
+set -e
+
+export PYTHONOPTIMIZE=


needs a comment on why it's disabled. Related to creation of .pyc/.pyo files? Strips comments that Pylint needs?

giorgiosironi · 2018-08-21T08:36:23Z

.gitignore

@@ -124,3 +124,6 @@ pip-selfcheck.json


 # End of https://www.gitignore.io/api/python
+
+# IDEs
+.vscode/


May be better suited to a global .gitignore

giorgiosironi

Some comments on versioning, documentation. The changes enforced by Pylint look benign so far. Needs a merge of master if not done recently.

This reverts commit f4d0448.

de-code · 2018-08-21T09:16:29Z

Still up-to-date with master. Would be good to get this merged.

giorgiosironi · 2018-08-21T10:41:05Z

.dockerignore

@@ -0,0 +1,2 @@
+**/*.pyc
+**/*.pyo


I always have to lookup this format, it works

Would be nice if they could just use the .gitignore format, and perhaps by default also use .gitignore (but I am sure there are many reasons against that idea).

giorgiosironi · 2018-08-21T10:48:00Z

Pipfile

@@ -5,6 +5,7 @@ name = "pypi"

 [dev-packages]
 pytest = "~=3.7.1"
+pylint = "2.*"


pkg_resources.extern.packaging.requirements.InvalidRequirement: Invalid requirement, parse error at "'.*'"

I think you need to use ~=2.1 which means >= 2.1, < 3.0

Okay, should have run pipenv lock as the docker build is ignoring the file. That's fixed now as well. (also updated the lock)

giorgiosironi

Only missing the correction to the pylint version, then we can merge

de-code added 9 commits August 16, 2018 16:25

added pylint dependency

21fe9c6

added project_tests.sh script

ca28a2b

Merge remote-tracking branch 'upstream/master' into code-checks

29de027

pass file pattern to pylint

2ffa6b1

merged from master

97da85e

add g++ to venv image (otherwise pylint can't be installed)

05cadff

add project_tests.sh to image

5fb3707

use sh instead of bash to run project tests

acf2bf0

changed path for python modules to test

8c8de81

de-code commented Aug 17, 2018

View reviewed changes

de-code added 9 commits August 17, 2018 18:10

run pytest as part of project tests

439284f

run project_tests.sh script as part of travis

7dd334e

added initial pylintrc configuration

dba8446

Merge remote-tracking branch 'upstream/master' into code-checks

6591d36

renamed ac to article_version

b559dae

moved standard import uuid up

269b590

added underscore in front of ping and testdb to avoid unused-variable…

5678c25

… warning

renamed database.db to DB to adhere to naming convention

8149f48

added id to good-names

13f4ae5

de-code commented Aug 20, 2018

View reviewed changes

.pylintrc

[BASIC]

good-names=

id

Copy link

Contributor Author

de-code Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if id is actually a good name, given that it's also a built-in function

de-code commented Aug 20, 2018

View reviewed changes

de-code added 3 commits August 20, 2018 11:25

added __init__.py for pylint

b0c98c1

include tests for pylint checks

c2aa530

fixed redefined-outer-name of client

e11b138

de-code commented Aug 20, 2018

View reviewed changes

de-code changed the title ~~[wip] pylint code checks~~ pylint code checks Aug 20, 2018

de-code mentioned this pull request Aug 20, 2018

Coding standard automated checks for Python projects libero/publisher#35

Closed

added .vscode to gitignore

f4d0448

de-code commented Aug 20, 2018

View reviewed changes

giorgiosironi reviewed Aug 20, 2018

View reviewed changes

de-code added 2 commits August 20, 2018 16:54

renamed project_tests.sh to project-tests.sh

32213d7

disable python optimization when running tests

54dde88

giorgiosironi reviewed Aug 21, 2018

View reviewed changes

de-code added 4 commits August 21, 2018 09:57

added comment re PYTHONOPTIMIZE

14e35c4

added .pyc and pyo to .dockerignore

c54cf8e

added major version to pylint

ebb6850

Revert "added .vscode to gitignore"

5ca6c82

This reverts commit f4d0448.

added major version to pylint (was accidently put on flask-sqlalchemy)

51064ba

giorgiosironi reviewed Aug 21, 2018

View reviewed changes

de-code added 4 commits August 21, 2018 11:51

fixed pylint dependency range

f03d327

updated pipenv lock

70fcfbe

used shorter pylint version format

56b6e00

updated lock again

1f4388a

giorgiosironi approved these changes Aug 21, 2018

View reviewed changes

giorgiosironi merged commit 19dab62 into libero:master Aug 21, 2018

de-code deleted the code-checks branch August 21, 2018 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pylint code checks #9

pylint code checks #9

de-code commented Aug 16, 2018 •

edited

Loading

de-code Aug 17, 2018

de-code Aug 17, 2018

de-code Aug 20, 2018

de-code Aug 20, 2018

de-code Aug 20, 2018

de-code Aug 20, 2018

de-code Aug 20, 2018

giorgiosironi Aug 21, 2018

de-code Aug 20, 2018

de-code Aug 20, 2018

de-code Aug 20, 2018

giorgiosironi Aug 20, 2018

de-code Aug 20, 2018

giorgiosironi Aug 20, 2018 •

edited

Loading

giorgiosironi Aug 20, 2018

de-code Aug 20, 2018

giorgiosironi Aug 20, 2018

de-code Aug 20, 2018

giorgiosironi Aug 21, 2018

giorgiosironi left a comment

giorgiosironi Aug 21, 2018

de-code Aug 21, 2018

giorgiosironi Aug 21, 2018

de-code Aug 21, 2018

giorgiosironi Aug 21, 2018

giorgiosironi Aug 21, 2018

giorgiosironi Aug 21, 2018

giorgiosironi left a comment

de-code commented Aug 21, 2018

giorgiosironi Aug 21, 2018

de-code Aug 21, 2018

giorgiosironi Aug 21, 2018

de-code Aug 21, 2018

giorgiosironi left a comment


		# TODO : add migrations
		# database setup (will use migration in the future)

		@@ -0,0 +1,2 @@
		*/.pyc
		*/.pyo

pylint code checks #9

pylint code checks #9

Conversation

de-code commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorgiosironi Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorgiosironi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorgiosironi left a comment

Choose a reason for hiding this comment

de-code commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorgiosironi left a comment

Choose a reason for hiding this comment

de-code commented Aug 16, 2018 •

edited

Loading

giorgiosironi Aug 20, 2018 •

edited

Loading