Skip to content

remove object base class, replace typing aliases - #885

Open
Khabib73 wants to merge 5 commits into
ydb-platform:mainfrom
Khabib73:issue/#884
Open

remove object base class, replace typing aliases#885
Khabib73 wants to merge 5 commits into
ydb-platform:mainfrom
Khabib73:issue/#884

Conversation

@Khabib73

@Khabib73Khabib73 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

These are not all the changes. I've kept it to the typing cleanup for now and will follow up with the rest separately. (to avoid making the diff too large)

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #884

@codecov

codecovBot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.36%. Comparing base (0b97adb) to head (c22ffb4).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@ Coverage Diff @@## main #885 +/- ##
==========================================
+ Coverage 82.33% 82.36% +0.02% 
==========================================
Files 99 99 Lines 12750 12753 +3 Branches 1242 1250 +8 ==========================================
+ Hits 10498 10504 +6 + Misses 1798 1795 -3 
Partials 454 454 
FlagCoverage Δ
integration80.19% <100.00%> (-0.02%)⬇️
unit48.57% <93.20%> (+0.18%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
ydb/_apis.py100.00% <100.00%> (ø)
ydb/_session_impl.py61.11% <100.00%> (ø)
ydb/_sp_impl.py54.54% <100.00%> (ø)
ydb/_tx_ctx_impl.py91.12% <100.00%> (ø)
ydb/_typing.py100.00% <100.00%> (ø)
ydb/_utilities.py78.80% <100.00%> (ø)
ydb/aio/query/pool.py94.73% <ø> (+0.10%)⬆️
ydb/aio/query/session.py92.50% <ø> (+3.75%)⬆️
ydb/aio/table.py80.32% <ø> (ø)
ydb/connection.py80.92% <100.00%> (ø)
... and 16 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Khabib73
Khabib73 marked this pull request as draft August 27, 2026 10:50
@Khabib73
Khabib73 marked this pull request as ready for review August 27, 2026 11:37
@vgvoleg
vgvoleg requested a balanced review from CopilotAugust 27, 2026 11:48

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Modernizes legacy Python syntax for issue #884 without intended runtime changes.

Changes:

  • Replaces typing collection aliases with built-in generics.
  • Removes redundant Python 2-era object inheritance.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.

Show a summary per file
FileDescription
ydb/tracing.pyModernizes tracing annotations.
ydb/topic.pyModernizes topic API annotations.
ydb/table.pyModernizes table classes and annotations.
ydb/settings.pyModernizes header annotation.
ydb/scripting.pyRemoves redundant object bases.
ydb/scheme.pyRemoves redundant object bases.
ydb/resolver.pyModernizes resolver classes.
ydb/query/session.pyModernizes explain return type.
ydb/query/pool.pyModernizes query pool return types.
ydb/query/base.pyModernizes callback storage annotation.
ydb/pool.pyModernizes pool annotations.
ydb/observability/metrics.pyModernizes metrics annotations.
ydb/import_client.pyRemoves redundant object base.
ydb/export.pyRemoves redundant object base.
ydb/driver.pyModernizes driver class and annotations.
ydb/credentials.pyRemoves redundant object base.
ydb/connection.pyModernizes connection classes and annotations.
ydb/_utilities.pyModernizes utility classes and annotations.
ydb/_typing.pyModernizes the RPC argument tuple alias.
ydb/_tx_ctx_impl.pyRemoves redundant object base.
ydb/_sp_impl.pyRemoves redundant object base.
ydb/_session_impl.pyRemoves redundant object bases.
ydb/_apis.pyModernizes service descriptor classes.
Suppressed comments (2)

ydb/table.py:1501

  • The matching public async method still declares rename_items as List[Tuple[str, str]] at ydb/aio/table.py:387-390. Please mirror this annotation cleanup in the async API so sync and async signatures remain aligned.
 rename_items: list[tuple[str, str]],

ydb/query/pool.py:302

  • The async explain_with_retries counterpart still returns Union[str, Dict[str, Any]] at ydb/aio/query/pool.py:246-253. Please mirror this conversion in the async method so runtime annotations and the sync/async public APIs remain consistent.
 ) -> Union[str, dict[str, Any]]:

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadydb/table.py
Comment on lines +1361 to +1365
add_columns: Optional[list["ydb.Column"]] = None,
drop_columns: Optional[list[str]] = None,
settings: Optional["settings_impl.BaseRequestSettings"] = None,
alter_attributes: Optional[Optional[Dict[str, str]]] = None,
add_indexes: Optional[List["ydb.TableIndex"]] = None,
drop_indexes: Optional[List[str]] = None,
alter_attributes: Optional[Optional[dict[str, str]]] = None,
add_indexes: Optional[list["ydb.TableIndex"]] = None,
Comment threadydb/table.py
def copy_tables(
self,
source_destination_pairs: List[Tuple[str, str]],
source_destination_pairs: list[tuple[str, str]],
Comment threadydb/query/session.py
*,
result_format: QueryExplainResultFormat = QueryExplainResultFormat.STR,
) -> Union[str, Dict[str, Any]]:
) -> Union[str, dict[str, Any]]:
Comment threadydb/query/pool.py
pool_id: Optional[str] = None,
**kwargs,
) -> List[convert.ResultSet]:
) -> list[convert.ResultSet]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@Khabib73

Khabib73 commented Aug 27, 2026

Copy link
Copy Markdown
ContributorAuthor

Can you run copilot review again?

@vgvoleg

Copy link
Copy Markdown
Member

I'm ok with huge diff, let's do all refactoring in one PR

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

@Khabib73

Khabib73 commented Aug 27, 2026

Copy link
Copy Markdown
ContributorAuthor

@vgvoleg How about replacing Union[T1, T2] with T1 | T2?

@Khabib73

Khabib73 commented Aug 28, 2026

Copy link
Copy Markdown
ContributorAuthor

Also, I think we should replace Optional[T] with T | None since it's the same as Union[T, None]. But it's a big job, so I'm waiting for your confirmation before I start. Does that sound good to you?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Khabib73@vgvoleg