About pylint configuration

[dalf]

Here a command to get some statistics:
$ git grep "pylint: disable=" | cut -f2 -d\# | cut -f2 -d\= | tr , '\n' | sed 's/^ *//' | sort | uniq -c | sort -nr

• cut -f2 -d\#: the comment at the end of line (can be broken)
• cut -f2 -d\= : after pylint: disable=
• tr , '\n' : split coma separated values into multi lines
• sed 's/^ *//': trim spaces

Result:

     37 missing-function-docstring
     13 no-member
      9 invalid-name
      9 global-statement
      9 assigning-non-slot
      7 undefined-variable
      7 missing-module-docstring
      7 broad-except
      6 NOQA 
      6 attribute-defined-outside-init
      4 unused-import
      4 E1101
      3 protected-access
      3 no-self-use
      3 consider-using-with
      2 W0102
      2 unused-argument
      2 too-many-arguments
      2 C0116
      2 access-member-before-definition
      1 wrong-import-position
      1 W0223  
      1 useless-object-inheritance
      1 too-many-boolean-expressions
      1 stop-iteration-return
      1 see https://github.com/encode/httpx/blob/e05a5372eb6172287458b37447c30f650047e1b8/httpx/_transports/default.py
      1 R0903
      1 pointless-string-statement
      1 no-name-in-module
      1 missing-class-docstring
      1 inconsistent-return-statements
      1 import-outside-toplevel)
      1 from https://github.com/encode/httpcore/blob/4b662b5c42378a61e54d673b4c949420102379f5/httpcore/_backends/asyncio.py
      1 E0611
      1 E0401 C0413
      1 E0401
      1 C0301
      1 arguments-differ

.pylintrc to update (?)

• missing-function-docstring: 17 occurrences for searx.engines.* . Currently the pattern # pylint: disable=missing-function-docstring at the top of the file doesn't help in my opinion. Documentation about the request and response functions doesn't help. But documentation about custom functions can help. It is only my opinion.
• unused-import: all related to import _fetch_supported_languages, supported_languages_url: I would disabled unused-import for searx.engines.* or find a way to tell pylint that _fetch_supported_languages, supported_languages_url are actually used.
• global-statement: do we really need a reminder about global variable usage?

actual issue:

• invalid-name is actually not required in searx.engines.* (7 occurrences / 9).
• no-member: related to preferences.py (see constructor)
• assigning-non-slot: all related to request.something = , actually it should be g.something =.

manage script issue:

• undefined-variable is not required since there is PYLINT_ADDITIONAL_BUILTINS_FOR_ENGINES, but for some reason it is ignored: it is a bug.

[return42]

.pylintrc to update (?)

No. :) ... the intention must be "improve the code" .. disabling messages for all files means: we do not want to improve the code / even in new files (new development).

Engines and Plugins are special and need a different treatment.

Currently the pattern # pylint: disable=missing-function-docstring at the top of the file doesn't help in my opinion.

It will not help for existing files, except we add documentation to existing functions, but this is a hard job and includes the risk to fail.

I would disabled unused-import for searx.engines.* or find a way to tell pylint that _fetch_supported_languages, supported_languages_url are actually used.

Engines and Plugins have name injections and need their own .pylintrc file where e.g. things names like PYLINT_ADDITIONAL_BUILTINS_FOR_ENGINES="supported_languages,language_aliases" are defined.

actual issue: invalid-name is actually not required in searx.engines.* (7 occurrences / 9).

For me a line on the top of a python file like::

# pylint: disable=invalid-name, missing-function-docstring

indicates, that this file is much improvable.

Documentation about the request and response functions doesn't help.

It will help to maintain .. by example: this is an example for a usefull module-doc-string of an engine:

searxng/searx/engines/google_videos.py

Lines 5 to 20 in b1da97d

	For detailed description of the *REST-full* API see: `Query Parameter
	Definitions`_. Not all parameters can be appied.
	.. _admonition:: Content-Security-Policy (CSP)
	This engine needs to allow images from the `data URLs`_ (prefixed with the
	``data:` scheme).::
	Header set Content-Security-Policy "img-src 'self' data: ;"
	.. _Query Parameter Definitions:
	https://developers.google.com/custom-search/docs/xml_results#WebSearch_Query_Parameter_Definitions
	.. _data URLs:
	https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs
	"""

On the long term we can include such doc-strings into the docs-build, but ATM we only have rare useful doc-strings. I haven't found one for the request & response function. But here is an example where such a doc-string is needed to understand why a http header is added:

searxng/searx/engines/wolframalpha_api.py

Lines 44 to 46 in b1da97d

	def request(query, params):
	params['url'] = search_url.format(query=urlencode({'input': query}), api_key=api_key)
	params['headers']['Referer'] = site_url.format(query=urlencode({'i': query}))

no-member: related to preferences.py (see constructor)
assigning-non-slot: all related to request.something = , actually it should be g.something =.

We will always have situations that an automated Lint process cannot capture from a technical point of view. Here, we have to think about workarounds. Adding "pylint-dissable" remarks might be annoying sometimes, but it is one workaround, that is available everywhere.

Over all:

For me, the .pylintrc file is the "holly grail". This file defines the quality I strive for.

For me improving code-quality is an ongoing process. Comments disabling some pylint messages are a warning sign that something can be improved here. The improvable code could remain over a long time. If there comes a day this code needs to be touched for some other reasons (e.g. a bugfix) I will also fix the lacks reported by pylint.

Since I can't lint only a hunk of a file, I first lint the whole file (adding comments to disable some linter messages). After this, I implement my new code in this file. This assures that my new code is conform to my "holly grail".

[return42]

manage script issue:

• undefined-variable is not required since there is PYLINT_ADDITIONAL_BUILTINS_FOR_ENGINES, but for some reason it is ignored: it is a bug.

For me it works (edit to PYLINT_ADDITIONAL_BUILTINS_FOR_ENGINES="" and you will get some error messages.

BTW, I was looking here for anything I can improve, but don't see what can be improved. What do you think, is there anything left we can improve to to get this issue closed? .. do you miss something?

[dalf]

do you miss something?

My bad, it works without # pylint: disable=undefined-variable.

So why add # pylint: disable=undefined-variable in searx/engines/*.py ?

[return42]

So why add # pylint: disable=undefined-variable in searx/engines/*.py ?

We have engines tagged with # lint: pylint. In the past we needed to ignore undefined-variable.

Since 7b235a1 (see line 591) this should no longer bee needed .. I will send a PR which cleans up this.

[dalf]

Another point: what about missing-function-docstring in searx/engines/*.py ?

Documentation about the request and response functions doesn't help.

It will help to maintain .. by example: this is an example for a usefull module-doc-string of an engine:

Do we need pylint to require documentation? If it makes sense to add documentation to a function we can add it without pylint especially since:

• this is the engine code: most of the time it is just request, response and sometimes init
• there is a review (pull request)

[return42]

If it makes sense to add documentation to a function we can add it without pylint especially since:

• this is the engine code: most of the time it is just request, response and sometimes init
• there is a review (pull request)

IMO engines are always very special and I would enforce developers to document the engine. We have done this already for some engines:

• https://searxng.github.io/searxng/src/index.html

From this point of view I think it is good to raise a pylint messages when an engine is not documented (for historical or whatever reason) but tagged with a # lint: pylint

For those engine files not tagged the by # lint: pylint we have additional disable options

PYLINT_SEARX_DISABLE_OPTION="\
I,C,R,\
W0105,W0212,W0511,W0603,W0613,W0621,W0702,W0703,W1401,\
E1136"

Where we disable C what includes C0116 missing-function-docstring

[dalf]

I would enforce developers to document the engine.

Why when in most of the cases there is no documentation to add?

What will happen:

• either ignore # lint: pylint
• or copy paste pylint: disable=missing-function-docstring from another engine.

and we go back to the review.

[return42]

Why when in most of the cases there is no documentation to add?

I have a different opinion, I think there is always something to document about the engine and how it works.

or copy paste pylint: disable=missing-function-docstring from another engine.

By example: if I develop a complete new engine module and want to have a strict pylint test ...

searxng/manage

Lines 604 to 607 in 9ef7f38

	build_msg TEST "[pylint] \$PYLINT_FILES"
	pyenv.cmd python ${PYLINT_OPTIONS} ${PYLINT_VERBOSE} \
	--additional-builtins="${PYLINT_ADDITIONAL_BUILTINS_FOR_ENGINES}" \
	"${PYLINT_FILES[@]}"

I tag the file by # lint: pylint ...

searxng/manage

Lines 19 to 29 in 9ef7f38

	pylint.FILES() {
	# List files tagged by comment:
	#
	# # lint: pylint
	#
	# These py files are linted by test.pylint(), all other files are linted by
	# test.pep8()
	grep -l -r --include \*.py '^#[[:blank:]]*lint:[[:blank:]]*pylint' searx searx_extra tests
	}

If do not want to have a strict pylint test I leave the file untagged. This file will then be tested with additional --disable="${PYLINT_SEARX_DISABLE_OPTION}" .. this is the case for most of the historical existing engines

searxng/manage

Lines 609 to 613 in 9ef7f38

	build_msg TEST "[pylint] searx/engines"
	pyenv.cmd python ${PYLINT_OPTIONS} ${PYLINT_VERBOSE} \
	--disable="${PYLINT_SEARX_DISABLE_OPTION}" \
	--additional-builtins="${PYLINT_ADDITIONAL_BUILTINS_FOR_ENGINES}" \
	searx/engines

If I have tagged my engine by by # lint: pylint it will be tested strictly. Where strictly means also, a documentation is needed otherwise disable this warning message by #pylint: disable=missing-function-docstring

[dalf]

My opinion:

• If the idea is to welcome contributors, we should make their life easy. The more obstacles between a contributor and a PR there are, the less we will have contributions.
• If we need documentation, there is no need to pylint to tap on shoulders.

I'm just talking about the docstring rules for the engines, not the whole pylint configuration.

But I understand this is not your opinion.
I leave this issue closed.

[return42]

If we need documentation, there is no need to pylint to tap on shoulders.

OK, that is an argument I can understand .. I reopen this issue and will send a PR which cleans up the disable=missing-function-docstring ..