Pylint integration updates #643

macohen · 2024-01-08T15:10:35Z

Description

extracted a method for per-folder linting
created a feature flag for newer dynamic pylint so now lints can be fixed first locally and then enabled without forcing build failures
ignore opensearchpy for pylint
updated noxfile.lint_per_folder with type hints
enabled unspecified-encoding in pylint
added disable missing-function-docstring pragma to test_clients.py in test_async and test_server

Issues Resolved

Progress on #610

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mark Cohen <[email protected]>

…l continue to work on this before committing setup.cfg Signed-off-by: Mark Cohen <[email protected]>

…'t fail while work on docstrings continues Signed-off-by: Mark Cohen <[email protected]>

Signed-off-by: Mark Cohen <[email protected]>

…ns; added a utility to automatically add these ignore instructions to most functions that should be self-describing; rolled back some automatically generated code mistakenly changed Signed-off-by: Mark Cohen <[email protected]>

codecov · 2024-01-08T15:12:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3eba72c) 72.10% compared to head (107469c) 72.04%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
- Coverage   72.10%   72.04%   -0.07%     
==========================================
  Files          89       89              
  Lines        7949     7945       -4     
==========================================
- Hits         5732     5724       -8     
- Misses       2217     2221       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* fixed some lints; created a feature flag for newer dynamic pylint so now lints can be fixed first in legacy code and then enabled by multiple people * extracted a method for per-folder linting * updated noxfile.lint_per_folder with type hints * enabled unspecified-encoding in pylint * added disable missing-function-docstring pragma to test_clients.py in test_async and test_server * added more encodings to pass unspecified-encoding pylint tests * updated changelog Signed-off-by: Mark Cohen <[email protected]>

dblock

I htink all or some of this PR should be merged, the thing I am strongly against is adding # pylint: disable=missing-function-docstring in every test. That is a huge burden on test authors for no value at all, so how can this be avoided? More comments inline.

CHANGELOG.md

setup.cfg

noxfile.py

dblock

Looks better.

I am still against adding # pylint: disable=missing-function-docstring to all the test files. Can we undo that part and skip that linter for tests?

noxfile.py

removed the feature flag for pylint lint_per_folder fixed failures from mypy and pylint removed pylint MESSAGE CONTROL config from setup.cfg after relocating to lint_per_folder method Signed-off-by: Mark Cohen <[email protected]>

Signed-off-by: Mark Cohen <[email protected]>

test_opensearchpy/test_server/test_helpers/conftest.py

test_opensearchpy/test_async/test_server/test_helpers/test_faceted_search.py

saimedhi · 2024-01-16T18:14:04Z

test_opensearchpy/test_async/test_server/test_helpers/conftest.py

+    yields the version of the OpenSearch cluster
+    :param client:
+    :return:
+    """


missing param description

I'm also adding a pylint extension to add missing-param-doc (when a parameter is missing from the docstring) and differing-param-doc (when a parameter is different in the function definition and the docstring).

updated some docstrings to correct parameters removed pylint from setup.cfg Signed-off-by: Mark Cohen <[email protected]>

dblock

@saimedhi what remains to be done to get this merged? Let's get an iteration in?

Signed-off-by: Mark Cohen <[email protected]>

saimedhi · 2024-01-17T21:29:36Z

Thank you, @macohen , for your contribution! I'll thoroughly review the PR today and aim to merge it soon. I've observed a failing integration test, so I'm rerunning it to determine if it's a flaky test.

macohen · 2024-01-17T21:35:03Z

Thank you, @macohen , for your contribution! I'll thoroughly review the PR today and aim to merge it soon. I've observed a failing integration test, so I'm rerunning it to determine if it's a flaky test.

Thanks @saimedhi. That's the test against main?

FAILED test_opensearchpy/test_server/test_rest_api_spec.py::test_rest_api_spec[OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/indices/stats/50_noop_update[0]] - opensearchpy.exceptions.ConnectionTimeout: ConnectionTimeout caused by - ReadTimeoutError(HTTPConnectionPool(host='localhost', port=9200): Read timed out. (read timeout=3))

I'm not sure I've ever seen that test succeed in this PR.

saimedhi · 2024-01-17T21:50:22Z

Thank you, @macohen , for your contribution! I'll thoroughly review the PR today and aim to merge it soon. I've observed a failing integration test, so I'm rerunning it to determine if it's a flaky test.

Thanks @saimedhi. That's the test against main?

FAILED test_opensearchpy/test_server/test_rest_api_spec.py::test_rest_api_spec[OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/indices/stats/50_noop_update[0]] - opensearchpy.exceptions.ConnectionTimeout: ConnectionTimeout caused by - ReadTimeoutError(HTTPConnectionPool(host='localhost', port=9200): Read timed out. (read timeout=3))

I'm not sure I've ever seen that test succeed in this PR.

Yes, it is test against main or unreleased OpenSearch. I will check, if the failing test is related to this PR or not.

dblock

Here's why the test fails.

dblock · 2024-01-17T22:12:47Z

test_opensearchpy/test_async/test_server/test_helpers/conftest.py

+    :param client:
+    :return:
+    """
+    info = client.info()


This is missing an await, was edited out, so you get the error.

that was also a dumb copy/paste error if you look at the wider scope. fixing. thanks, @dblock.

Signed-off-by: Mark Cohen <[email protected]>

saimedhi

A recently added test is failing. Please add it to the skip list here. Apart from that, I'm okay with merging this code. A few minor points below can be addressed in a follow-up PR if interested. Tested by running samples and generator, and they are working fine.

opensearchpy/compat.py

test_opensearchpy/run_tests.py

utils/generate_api.py

test_opensearchpy/test_async/test_server/test_helpers/test_actions.py

test_opensearchpy/test_async/test_server/test_helpers/test_faceted_search.py

saimedhi · 2024-01-18T21:22:42Z

Hi @macohen, in your latest commit, could you please remove the changes causing test failures? If not, consider excluding that commit and let's proceed with merging this code. We can address the other changes in a follow-up PR. Apologies for the delay.

macohen · 2024-01-19T13:57:37Z

Hi @macohen, in your latest commit, could you please remove the changes causing test failures? If not, consider excluding that commit and let's proceed with merging this code. We can address the other changes in a follow-up PR. Apologies for the delay.

Thanks. I was working on figuring out how to run the two tasks that are failing locally so I can reproduce the failures. I thought I needed to check out the appropriate branch of opensearch (2.0, main) and run the commands in the workflow, but I haven't been able to reproduce. If you have any guidance there, that would be helpful. Also, I will update this comment in about an hour with the command I'm running...

dblock

I opened #651 with a trivial change and it failed in these integration tests the same way. @macohen open an issue please, fix the TODOs ask and we'll merge this.

test_opensearchpy/test_async/test_server/test_helpers/conftest.py

renamed test_opensearchpy.test_async.test_server.test_helpers.conftest.setup_ubq_tests to setup_update_by_query_tests added OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/indices/stats/50_noop_update[0] to skip tests list run_tests.py catches a CalledProcessError when the git repo already exists and the command to add the origin fails in fetch_opensearch_repo() Signed-off-by: Mark Cohen <[email protected]>

macohen · 2024-01-19T18:23:59Z

I opened #651 with a trivial change and it failed in these integration tests the same way. @macohen open an issue please, fix the TODOs ask and we'll merge this.

Opened #653, pushed an update with the failing test skipped and all checks pass now! Thanks to @saimedhi and @dblock for all the help and patience. :)

macohen added 9 commits December 18, 2023 20:57

updated files with docstrings to pass pylint

10a2b4f

Signed-off-by: Mark Cohen <[email protected]>

updated samples to prepare for enabling missing-docstring linter; wil…

9b8b830

…l continue to work on this before committing setup.cfg Signed-off-by: Mark Cohen <[email protected]>

removed missing-function-docstring from setup.cfg so the linter doesn…

bc5aac1

…'t fail while work on docstrings continues Signed-off-by: Mark Cohen <[email protected]>

corrected unnecessary return docstring values

78887f6

Signed-off-by: Mark Cohen <[email protected]>

fixing failure in 'black' on reformatting

8752a16

Signed-off-by: Mark Cohen <[email protected]>

updated utils to pass missing-function-docstring tests

435948b

Signed-off-by: Mark Cohen <[email protected]>

Merge branch 'opensearch-project:main' into main

a2fa5e7

Merge branch 'opensearch-project:main' into main

dabaa59

macohen requested review from VachaShah, dblock, harshavamsi, axeoman, Shephalimittal, saimedhi and florianvazelle as code owners January 8, 2024 15:10

macohen force-pushed the tests-doc-strings branch 4 times, most recently from 206dff7 to 1680f34 Compare January 8, 2024 16:06

macohen force-pushed the tests-doc-strings branch from 1680f34 to d26eba5 Compare January 8, 2024 16:08

dblock requested changes Jan 8, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

setup.cfg Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

dblock requested changes Jan 9, 2024

View reviewed changes

dblock reviewed Jan 9, 2024

View reviewed changes

noxfile.py Show resolved Hide resolved

updated CHANGELOG.md entry

0a2302f

removed the feature flag for pylint lint_per_folder fixed failures from mypy and pylint removed pylint MESSAGE CONTROL config from setup.cfg after relocating to lint_per_folder method Signed-off-by: Mark Cohen <[email protected]>

macohen force-pushed the tests-doc-strings branch 3 times, most recently from 066aa7b to 2a03f3e Compare January 11, 2024 14:09

macohen force-pushed the tests-doc-strings branch from 040b0d0 to 6be4396 Compare January 13, 2024 23:07

removed pylint ignore missing-function-docstring

fcc65a3

Signed-off-by: Mark Cohen <[email protected]>

macohen force-pushed the tests-doc-strings branch from 6be4396 to fcc65a3 Compare January 13, 2024 23:23

saimedhi requested changes Jan 16, 2024

View reviewed changes

added pylint.extensions.docparams plugin

2265ad3

updated some docstrings to correct parameters removed pylint from setup.cfg Signed-off-by: Mark Cohen <[email protected]>

dblock previously approved these changes Jan 17, 2024

View reviewed changes

macohen mentioned this pull request Jan 17, 2024

[PROPOSAL] Expanding Pylint in opensearchpy package #647

Closed

added four lints for opensearchpy/

d9c0874

Signed-off-by: Mark Cohen <[email protected]>

macohen dismissed dblock’s stale review via d9c0874 January 17, 2024 21:25

dblock reviewed Jan 17, 2024

View reviewed changes

adding await back to client.info() call

ffa80b3

Signed-off-by: Mark Cohen <[email protected]>

saimedhi previously approved these changes Jan 18, 2024

View reviewed changes

macohen dismissed saimedhi’s stale review via 6450f03 January 18, 2024 20:58

macohen force-pushed the tests-doc-strings branch 4 times, most recently from 030d5ae to d8247a0 Compare January 19, 2024 01:34

This was referenced Jan 19, 2024

Renamed variables in benchmark code. #650

Closed

Removed unnecessary line in README. #651

Closed

dblock requested changes Jan 19, 2024

View reviewed changes

test_opensearchpy/test_async/test_server/test_helpers/conftest.py Outdated Show resolved Hide resolved

macohen force-pushed the tests-doc-strings branch from d8247a0 to 107469c Compare January 19, 2024 18:14

dblock approved these changes Jan 19, 2024

View reviewed changes

dblock merged commit 0ddbf8c into opensearch-project:main Jan 19, 2024
54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pylint integration updates #643

Pylint integration updates #643

macohen commented Jan 8, 2024

codecov bot commented Jan 8, 2024 •

edited

Loading

dblock left a comment

dblock left a comment

saimedhi Jan 16, 2024

macohen Jan 17, 2024

dblock left a comment

saimedhi commented Jan 17, 2024

macohen commented Jan 17, 2024

saimedhi commented Jan 17, 2024 •

edited

Loading

dblock left a comment

dblock Jan 17, 2024 •

edited

Loading

macohen Jan 17, 2024

saimedhi left a comment

saimedhi commented Jan 18, 2024

macohen commented Jan 19, 2024

dblock left a comment

macohen commented Jan 19, 2024

Pylint integration updates #643

Pylint integration updates #643

Conversation

macohen commented Jan 8, 2024

Description

Issues Resolved

codecov bot commented Jan 8, 2024 • edited Loading

Codecov Report

dblock left a comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

saimedhi Jan 16, 2024

Choose a reason for hiding this comment

macohen Jan 17, 2024

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

saimedhi commented Jan 17, 2024

macohen commented Jan 17, 2024

saimedhi commented Jan 17, 2024 • edited Loading

dblock left a comment

Choose a reason for hiding this comment

dblock Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

macohen Jan 17, 2024

Choose a reason for hiding this comment

saimedhi left a comment

Choose a reason for hiding this comment

saimedhi commented Jan 18, 2024

macohen commented Jan 19, 2024

dblock left a comment

Choose a reason for hiding this comment

macohen commented Jan 19, 2024

codecov bot commented Jan 8, 2024 •

edited

Loading

saimedhi commented Jan 17, 2024 •

edited

Loading

dblock Jan 17, 2024 •

edited

Loading