Python code cleanup #2573

prateek-kumar-improving · 2024-11-02T00:19:23Z

Issue link

This Pull Request is linked to issue (URL): #2429

PR description

This is a follow up PR based on a thorough review of the Python VSS module code to improve the code quality of the final code sent into the release. It includes the following changes:

Documentation fixes, test case refactoring and adding binary tests.

Move all tests to common file. As suggested during one of the code reviews.
Files removed: python/python/tests/tests_server_modules/search/test_ft_create.py,
python/python/tests/tests_server_modules/search/test_ft_search.py,
python/python/tests/tests_server_modules/search/test_ft_dropindex.py
All tests moved to python/python/tests/tests_server_modules/test_ft.py
Examples section in documentation - replace : with # in all commands
Remove all See statements like: See FtCreateOptions. This is not required here because unlike JAVA it does not link to anything. It is redundant code because the class name is present in params section the documentation and this is self explanatory.
Add tests for TEncodable(binary data) for all commands. Added for some commands already, maybe 3-4 commands were missed.
Python: Fix nit in this PR: FT.EXPLAIN and FT.EXPLAINCLI commands added #2508
Remove docs for overloaded methods in VSS module wherever necessary. Example: https://github.com/valkey-io/valkey-glide/pull/2530/files#r1819482800

Typo fixes, variable names consistency fixes, function name consistency fixes:

Typo in FtSearchOptions class name. Currently it is FtSeachOptions. Corrected the typo in the import statement and throughout the code by changing FtSeachOptions to FtSearchOptions in python/python/glide/__init__.py and python/python/glide/async_commands/server_modules/ft.py [1] [2] [3] [4].
Fix variable names in test cases to be underscore. Based on an internal discussion. Renamed variables for consistency, changing indexName to index_name in multiple functions in python/python/glide/async_commands/server_modules/ft.py [1] [2] [3] [4] [5] [6] [7] [8] [9] [10].
Change all function names to use underscore in Python. Example: Change toArgs() to to_args(). This is already done for FT.AGGREGATE. Fix for others. Updated method names from toArgs to to_args for consistency in python/python/glide/async_commands/server_modules/ft_options/ft_create_options.py [1] [2] [3] [4] [5] [6].
Corrected a typo in class name FtSeachKeywords to FtSearchKeywords in python/python/glide/async_commands/server_modules/ft_options/ft_constants.py.
Added a changelog entry for the Python code cleanup in CHANGELOG.md.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Commits will be squashed upon merging.

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

avifenesh · 2024-11-02T10:40:08Z

python/python/glide/async_commands/server_modules/ft.py

        schema (List[Field]): Fields to populate into the index. Equivalent to `SCHEMA` block in the module API.
-        options (Optional[FtCreateOptions]): Optional arguments for the FT.CREATE command. See `FtCreateOptions`.
+        options (Optional[FtCreateOptions]): Optional arguments for the FT.CREATE command.


Why do you remove the see?

In case of python, unlike JAVA and Node, this statement does not link to the actual class and is therefore not adding any extra value.

We need to find a python doc validator and generator. This will define doc format and then we can restore all see to ensure links are properly rendered in HTML docs.

avifenesh · 2024-11-02T10:41:41Z

python/python/glide/async_commands/server_modules/ft_options/ft_create_options.py

-        """
-        args = super().toArgs()
+    def to_args(self) -> List[TEncodable]:
+        args = super().to_args()


Why removing docs?
You can change to dev docs and leave something simple

This is an inherited class, and the doc from the main abstract class for this method is available when we hover over the method in IDE. This is the feedback I got when I discussed with others about this.
This was first mentioned in this PR comment: https://github.com/valkey-io/valkey-glide/pull/2530/files#r1819482800

I can change this back, if you think for some reason we need this.

If the docs in the abstract are sufficient, no need

avifenesh · 2024-11-02T10:43:01Z

python/python/tests/tests_server_modules/test_ft.py

-                + ' incline! They may be at the age when a 27.5\\" wheel bike is just too clumsy'
-                + ' coming off a 24\\" bike. The Hillcraft 26 is just the solution they need!",'
-                + ' "condition": "used"}',
+                '{"brand": "Bicyk", "model": "Hillcraft", "price": 1200, "condition": "used"}',


Upon review of the code across all languages, I found this has already been removed from the JAVA integ tests, I want to keep everything consistent across languages.
The reason for removal from JAVA: It is a JSON key that is not used for anything during testing and is adding no value by keeping it in the record.

avifenesh · 2024-11-02T10:44:25Z

Issue link

This Pull Request is linked to issue (URL): #2429

Checklist

Before submitting the PR make sure the following are checked:

* [x]  This Pull Request is related to one issue.

* [x]  Commit message has a detailed description of what changed and why.

* [x]  Tests are added or updated.

* [x]  CHANGELOG.md and documentation files are updated.

* [x]  Destination branch is correct - main or release

* [x]  Commits will be squashed upon merging.

The issue doesn't mention the reasoning for this PR, can you elaborate on the goal and actions?

avifenesh · 2024-11-02T10:46:12Z

Issue link

This Pull Request is linked to issue (URL): #2429

Checklist

Before submitting the PR make sure the following are checked:

* [x]  This Pull Request is related to one issue.

* [x]  Commit message has a detailed description of what changed and why.

* [x]  Tests are added or updated.

* [x]  CHANGELOG.md and documentation files are updated.

* [x]  Destination branch is correct - main or release

* [x]  Commits will be squashed upon merging.

Lazy blueprint for elaborating, you can edit with the real details:

This pull request focuses on code cleanup and consistency improvements in the Python codebase, specifically for the glide module. The changes include fixing typos, renaming variables for consistency, and updating method names to follow a uniform naming convention.

Code Cleanup and Consistency Improvements:

Corrected a typo in the import statement and throughout the code by changing FtSeachOptions to FtSearchOptions in python/python/glide/__init__.py and python/python/glide/async_commands/server_modules/ft.py [1] [2] [3] [4].
Renamed variables for consistency, changing indexName to index_name in multiple functions in python/python/glide/async_commands/server_modules/ft.py [1] [2] [3] [4] [5] [6] [7] [8] [9] [10].
Updated method names from toArgs to to_args for consistency in python/python/glide/async_commands/server_modules/ft_options/ft_create_options.py [1] [2] [3] [4] [5] [6].
Corrected a typo in class name FtSeachKeywords to FtSearchKeywords in python/python/glide/async_commands/server_modules/ft_options/ft_constants.py.
Added a changelog entry for the Python code cleanup in CHANGELOG.md.

prateek-kumar-improving · 2024-11-04T03:41:32Z

Issue link

This Pull Request is linked to issue (URL): #2429

Checklist

Before submitting the PR make sure the following are checked:
* [x]  This Pull Request is related to one issue.

* [x]  Commit message has a detailed description of what changed and why.

* [x]  Tests are added or updated.

* [x]  CHANGELOG.md and documentation files are updated.

* [x]  Destination branch is correct - main or release

* [x]  Commits will be squashed upon merging.
Lazy blueprint for elaborating, you can edit with the real details:

This pull request focuses on code cleanup and consistency improvements in the Python codebase, specifically for the glide module. The changes include fixing typos, renaming variables for consistency, and updating method names to follow a uniform naming convention.

Code Cleanup and Consistency Improvements:

Corrected a typo in the import statement and throughout the code by changing FtSeachOptions to FtSearchOptions in python/python/glide/__init__.py and python/python/glide/async_commands/server_modules/ft.py [1] [2] [3] [4].

Renamed variables for consistency, changing indexName to index_name in multiple functions in python/python/glide/async_commands/server_modules/ft.py [1] [2] [3] [4] [5] [6] [7] [8] [9] [10].

Updated method names from toArgs to to_args for consistency in python/python/glide/async_commands/server_modules/ft_options/ft_create_options.py [1] [2] [3] [4] [5] [6].

Corrected a typo in class name FtSeachKeywords to FtSearchKeywords in python/python/glide/async_commands/server_modules/ft_options/ft_constants.py.

Added a changelog entry for the Python code cleanup in CHANGELOG.md.

I have updated the details in the PR description. I will follow this format of description for PRs created for any improvement PRs I create for other languages as well. Thanks.

avifenesh

LGTM
We are freezing merges until CI is green again, then ill approve
Thanks!

python/python/tests/tests_server_modules/test_ft.py

prateek-kumar-improving added 3 commits November 1, 2024 17:00

Python code cleanup and test case update

8de8c9b

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Python code cleanup and test case update

7f4540e

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Python code cleanup and test case update

cc6b805

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving requested a review from a team as a code owner November 2, 2024 00:19

prateek-kumar-improving had a problem deploying to AWS_ACTIONS November 2, 2024 00:19 — with GitHub Actions Error

Update CHANGELOG.md

0b832a4

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving temporarily deployed to AWS_ACTIONS November 2, 2024 00:28 — with GitHub Actions Inactive

prateek-kumar-improving requested review from acarbonetto and Yury-Fridlyand November 2, 2024 00:30

prateek-kumar-improving mentioned this pull request Nov 2, 2024

Python: VSS commands support #2429

Closed

14 tasks

avifenesh reviewed Nov 2, 2024

View reviewed changes

prateek-kumar-improving mentioned this pull request Nov 4, 2024

Java: Update docs for VSS #2565

Merged

6 tasks

avifenesh reviewed Nov 4, 2024

View reviewed changes

Yury-Fridlyand approved these changes Nov 4, 2024

View reviewed changes

python/python/tests/tests_server_modules/test_ft.py Show resolved Hide resolved

Yury-Fridlyand added the python Python wrapper label Nov 4, 2024

acarbonetto merged commit 6e04584 into release-1.2 Nov 7, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python code cleanup #2573

Python code cleanup #2573

prateek-kumar-improving commented Nov 2, 2024 •

edited

Loading

avifenesh Nov 2, 2024

prateek-kumar-improving Nov 4, 2024

Yury-Fridlyand Nov 4, 2024

avifenesh Nov 2, 2024

prateek-kumar-improving Nov 4, 2024 •

edited

Loading

avifenesh Nov 4, 2024

avifenesh Nov 2, 2024

prateek-kumar-improving Nov 4, 2024 •

edited

Loading

avifenesh commented Nov 2, 2024

Issue link

Checklist

avifenesh commented Nov 2, 2024

Issue link

Checklist

prateek-kumar-improving commented Nov 4, 2024 •

edited

Loading

Issue link

Checklist

Code Cleanup and Consistency Improvements:

avifenesh left a comment •

edited

Loading

Python code cleanup #2573

Python code cleanup #2573

Conversation

prateek-kumar-improving commented Nov 2, 2024 • edited Loading

Issue link

PR description

Documentation fixes, test case refactoring and adding binary tests.

Checklist

avifenesh Nov 2, 2024

Choose a reason for hiding this comment

prateek-kumar-improving Nov 4, 2024

Choose a reason for hiding this comment

Yury-Fridlyand Nov 4, 2024

Choose a reason for hiding this comment

avifenesh Nov 2, 2024

Choose a reason for hiding this comment

prateek-kumar-improving Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

avifenesh Nov 4, 2024

Choose a reason for hiding this comment

avifenesh Nov 2, 2024

Choose a reason for hiding this comment

prateek-kumar-improving Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

avifenesh commented Nov 2, 2024

Issue link

Checklist

avifenesh commented Nov 2, 2024

Issue link

Checklist

Code Cleanup and Consistency Improvements:

prateek-kumar-improving commented Nov 4, 2024 • edited Loading

Issue link

Checklist

Code Cleanup and Consistency Improvements:

avifenesh left a comment • edited Loading

Choose a reason for hiding this comment

prateek-kumar-improving commented Nov 2, 2024 •

edited

Loading

prateek-kumar-improving Nov 4, 2024 •

edited

Loading

prateek-kumar-improving Nov 4, 2024 •

edited

Loading

prateek-kumar-improving commented Nov 4, 2024 •

edited

Loading

avifenesh left a comment •

edited

Loading