Skip to content

fix(api-client): code hygiene - #3867

Open
jaegeral wants to merge 31 commits into
google:masterfrom
jaegeral:2026-06-25-api-client
Open

fix(api-client): code hygiene#3867
jaegeral wants to merge 31 commits into
google:masterfrom
jaegeral:2026-06-25-api-client

Conversation

@jaegeral

@jaegeral jaegeral commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Removed from __future__ import unicode_literals from all api-client as we are py3 only.

Complete Parameter Coverage:
* Ensured every parameter in function signatures is documented in the Args: section.
* Added documentation for init parameters
* Added Args: sections to property setters (e.g., name, description, color, status, max_entries)
which were previously undocumented.

Added from __future__ import annotations in a lot of files to ensure typing works across various Python3 versions

Type Accuracy: improved type accuracy in docstrings

Added docstrings to most of api-client methods.

@jaegeral jaegeral self-assigned this Jun 25, 2026
@jaegeral jaegeral added API client Code Health Code health improvements labels Jun 25, 2026
@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request systematically adds and updates Google-style docstrings across the Timesketch Python API client, removes deprecated unicode_literals imports, and standardizes mock imports in tests. It also introduces a DEFAULT_CONFIG_SECTION constant. The review feedback points out several type and module path inaccuracies in the newly added docstrings, specifically regarding the types of client_id, event_id, and index_id (which should be str instead of int), and the module path for TimesketchApi (which should reference the client module rather than api).

Comment thread api_client/python/timesketch_api_client/sketch.py
Comment thread api_client/python/timesketch_api_client/sketch.py
Comment thread api_client/python/timesketch_api_client/client.py Outdated
Comment thread api_client/python/timesketch_api_client/sketch.py Outdated
Comment thread api_client/python/timesketch_api_client/sketch.py Outdated
jaegeral and others added 6 commits June 25, 2026 09:12
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces comprehensive type annotations, future annotations imports, and import cleanups across the Timesketch API client codebase. However, several issues were identified during the review: a critical syntax error in the Sketch.__init__ method due to a missing closing parenthesis and return type; a bug in story.py where stringifying block.data on a TextBlock outputs a dictionary string instead of the text; a potential crash in story.py when parsing empty content with json.loads(); and a regression in sketch.py where default field handling in export_events_stream explicitly requests zero fields instead of all fields.

Comment thread api_client/python/timesketch_api_client/sketch.py
Comment thread api_client/python/timesketch_api_client/story.py
Comment thread api_client/python/timesketch_api_client/story.py Outdated
Comment thread api_client/python/timesketch_api_client/sketch.py Outdated
jaegeral and others added 5 commits June 25, 2026 09:17
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request widely introduces type hints and annotations across the timesketch_api_client library to improve code clarity and enable static analysis. However, several critical issues were identified during the review. Most notably, merge or copy-paste errors in client.py and sketch.py have introduced duplicated imports, TYPE_CHECKING blocks, and method definitions that will cause syntax errors. Additionally, there is a potential TypeError in aggregation.py when accessing a potentially missing 'meta' key, a redundant bytes cast in crypto.py, and minor docstring mismatches. Finally, multiple instances of string formatting using .format() violate the repository's style guide, which prefers f-strings.

Comment thread api_client/python/timesketch_api_client/client.py Outdated
Comment thread api_client/python/timesketch_api_client/sketch.py Outdated
Comment on lines 360 to 361
meta = self.resource_data.get("meta")
meta["name"] = name

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The call self.resource_data.get("meta") can return None if the "meta" key does not exist in self.resource_data. The subsequent access meta["name"] = name would then raise a TypeError. To prevent this, you should provide a default empty dictionary, similar to how it's handled in the description setter on line 343.

Suggested change
meta = self.resource_data.get("meta")
meta["name"] = name
meta = self.resource_data.get("meta", {})

Comment thread api_client/python/timesketch_api_client/aggregation.py Outdated
Comment thread api_client/python/timesketch_api_client/aggregation.py Outdated
Comment thread api_client/python/timesketch_api_client/aggregation.py Outdated
Comment thread api_client/python/timesketch_api_client/aggregation.py Outdated
Comment thread api_client/python/timesketch_api_client/cli_input.py
backend=backends.default_backend(),
)
return base64.urlsafe_b64encode(kdf.derive(password))
return bytes(base64.urlsafe_b64encode(kdf.derive(password)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The base64.urlsafe_b64encode() function already returns a bytes object. The bytes() cast is redundant and can be removed for cleaner code.

Suggested change
return bytes(base64.urlsafe_b64encode(kdf.derive(password)))
return base64.urlsafe_b64encode(kdf.derive(password))

jaegeral and others added 8 commits June 25, 2026 10:09
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request primarily adds type hints, f-strings, and updated docstrings across various modules of the Timesketch API client to improve code quality and maintainability. The code review feedback highlights several issues that need to be addressed: potential TypeError crashes in aggregation.py when self.resource_data is None, duplicated if TYPE_CHECKING: blocks in client.py and sketch.py, and consecutively duplicated lines of code in sketch.py for retrieving the sketch name and processing permissions.

Comment thread api_client/python/timesketch_api_client/aggregation.py Outdated
Comment thread api_client/python/timesketch_api_client/aggregation.py Outdated
Comment thread api_client/python/timesketch_api_client/client.py Outdated
Comment thread api_client/python/timesketch_api_client/sketch.py Outdated
Comment thread api_client/python/timesketch_api_client/sketch.py Outdated
Comment thread api_client/python/timesketch_api_client/sketch.py Outdated
jaegeral and others added 6 commits June 25, 2026 12:43
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@jaegeral
jaegeral marked this pull request as ready for review June 25, 2026 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API client Code Health Code health improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants