search

opensearch-project / opensearch-py

Python Client for OpenSearch
https://opensearch.org/docs/latest/clients/python/
Apache License 2.0
329 stars 166 forks source link

[FEATURE] Enable pylint defaults #610

Open dblock opened 9 months ago

dblock commented 9 months ago

Is your feature request related to a problem?

We enabled pylint in #590. Enable the remaining lints. Here are some top ones.

$ python -m pylint setup.py noxfile.py opensearchpy/ test_opensearchpy/ utils/ samples/ benchmarks/ docs/ | grep '(' | grep '-' | rev | cut -d '(' -f1 | rev | cut -d ')' -f1 | sort | uniq -c | sort -r

1304 missing-function-docstring
 540 missing-class-docstring
 484 protected-access
 477 duplicate-code
 223 no-member
 203 missing-module-docstring
 143 unused-argument
 101 consider-using-f-string
  87 super-with-arguments
  78 too-few-public-methods
  77 redefined-builtin
  69 line-too-long
  68 too-many-arguments
  66 redefined-outer-name
  48 import-outside-toplevel
  41 useless-object-inheritance
  35 unused-variable
  32 unexpected-keyword-arg
  25 raise-missing-from
  24 too-many-locals
  23 unnecessary-dunder-call
  20 wrong-import-position
  18 too-many-public-methods
  18 invalid-unary-operand-type
  18 attribute-defined-outside-init
  17 unspecified-encoding
  16 no-else-return
  16 invalid-overridden-method
  15 cyclic-import
  11 too-many-branches
  11 dangerous-default-value
  11 arguments-renamed
  10 pointless-statement

What solution would you like?

Enable lints in setup.cfg one-by-one.

macohen commented 9 months ago

So, to confirm, when you say one-by-one, it's this approach:

  1. add a lint
  2. run pylint
  3. fix ALL the failures
  4. go to 1 until out of lints or I just can't anymore. ;)
macohen commented 9 months ago

Doing this incrementally, what do you think of breaking this down into an issue per lint? It becomes a much less daunting single task and can be picked up in parallel.

dblock commented 9 months ago

@macohen Yes, I don't expect all the lints to be enabled at once, that's a massive change.

macohen commented 8 months ago

Running utils/build_dists.py results in an error:

Installing collected packages: urllib3, six, idna, charset-normalizer, certifi, requests, python-dateutil, opensearch-py2 Successfully installed certifi-2023.11.17 charset-normalizer-3.3.2 idna-3.6 opensearch-py2-2.4.3 python-dateutil-2.8.2 requests-2.31.0 six-1.16.0 urllib3-1.26.18 $ /var/folders/33/jx0mw87156q2hmtrr_r82s7r0000gs/T/tmpko79lorv/venv/bin/python -c 'from opensearchpy2 import OpenSearch, Q' Traceback (most recent call last): File "", line 1, in File "/private/var/folders/33/jx0mw87156q2hmtrr_r82s7r0000gs/T/tmpko79lorv/venv/lib/python3.10/site-packages/opensearchpy2/init.py", line 46, in from .client import OpenSearch File "/private/var/folders/33/jx0mw87156q2hmtrr_r82s7r0000gs/T/tmpko79lorv/venv/lib/python3.10/site-packages/opensearchpy2/client/init.py", line 42, in from ..transport import Transport, TransportError File "/private/var/folders/33/jx0mw87156q2hmtrr_r82s7r0000gs/T/tmpko79lorv/venv/lib/python3.10/site-packages/opensearchpy2/transport.py", line 40, in from .serializer import DEFAULT_SERIALIZERS, Deserializer, JSONSerializer, Serializer File "/private/var/folders/33/jx0mw87156q2hmtrr_r82s7r0000gs/T/tmpko79lorv/venv/lib/python3.10/site-packages/opensearchpy2/serializer.py", line 41, in from .helpers.utils import AttrList File "/private/var/folders/33/jx0mw87156q2hmtrr_r82s7r0000gs/T/tmpko79lorv/venv/lib/python3.10/site-packages/opensearchpy2/helpers/utils.py", line 36, in from opensearchpy.exceptions import UnknownDslObject, ValidationException ModuleNotFoundError: No module named 'opensearchpy' Command exited incorrectly: should have been 0 was 256

Before I go down a rabbit hole, I think there may be some missing instruction about how to use this or it may not work correctly. I see a docstring:

A command line tool for building and verifying releases Can be used for building both 'opensearchpy' and 'opensearchpyX' dists. Only requires 'name' in 'setup.py' and the directory to be changed.

What's the intent of an opensearchpyX distribution? What name and directory should be changed?

dblock commented 8 months ago

@macohen I don't know anything about this one, I would start by checking whether this is dead code/used in any of our release GHA versions.

macohen commented 8 months ago

got that one. it is only called from GitHub Actions. Also, I've been adding a lot of "# pylint: disable=missing-function-docstring" instructions to any function starting with def test or async def test. All of those functions have pretty good descriptions as function names so I thought this would be a good exception. Even wrote a script for it that I'm adding to utils. Could use some improvements on multi-line function definitions...

I was thinking about adding to ignore in pylint in setup.cfg for the test_opensearchpy directory, but that seems too heavy handed...

macohen commented 8 months ago

Should the opensearchpy directory be ignored for pylint? That code is generated (I think) so maybe the generator needs to include the docstrings and other items.

dblock commented 8 months ago

I don't think tests need descriptions, so I would try to ignore test folders for that linter specifically, but not others. The generator definitely should include docstrings.

macohen commented 7 months ago

Tracking them here as I add:

macohen commented 7 months ago

@saimedhi is there a priority on the lints you'd like to see above? I've been tackling them based on what looks interesting to report on and not too much of a rabbit hole. I'll start updating my findings in the comment above as well.

saimedhi commented 7 months ago

Feel free to address the lints as you see fit; I don't have any specific priorities.