mypy: enable checking untyped code by jacalata · Pull Request #367 · tableau/tabcmd

jacalata · 2025-10-08T05:24:14Z

added explicit checks for None to avoid union typing
created a union for RequestOptionsType
made static methods into class methods
created a superclass for tests

c:\dev\tabcmd>mypy tests tests\commands\test_projects_utils.py:22: error: Argument 1 to "_parse_project_path_to_list" of "Server" has incompatible type "None"; expected "str" [arg-type] tests\commands\test_user_utils.py:76: error: Incompatible types in assignment (expression has type "UserItem | None", variable has type "UserItem") [assignment] tests\commands\test_user_utils.py:84: error: Incompatible types in assignment (expression has type "UserItem | None", variable has type "UserItem") [assignment] tests\e2e\language_tests.py:208: error: "OnlineCommandTest" has no attribute "_get_workbook" [attr-defined] tests\e2e\tests_integration.py:68: error: Name "logger" is not defined [name-defined] tests\e2e\tests_integration.py:99: error: Name "logger" is not defined [name-defined] tests\e2e\tests_integration.py:133: error: Name "__class__" is not defined [name-defined] tests\commands\test_session.py:206: error: Argument 1 to "_allow_prompt" has incompatible type "Namespace"; expected "Session" [arg-type] tests\commands\test_session.py:211: error: Argument 1 to "_allow_prompt" has incompatible type "Namespace"; expected "Session" [arg-type] tests\commands\test_session.py:216: error: Argument 1 to "_allow_prompt" has incompatible type "Namespace"; expected "Session" [arg-type] Found 10 errors in 5 files (checked 51 source files)

output with fixes > mypy tests Success: no issues found in 51 source files

github-actions · 2026-02-13T06:58:43Z

Coverage Report

File	Stmts	Miss	Cover	Missing
tabcmd
__main__.py	10	10	10	0%
tabcmd.py	15	15	15	0%
version.py	5	2	2	60%
tabcmd/commands
commands.py	10	10	10	0%
constants.py	77	18	18	77%
server.py	130	20	20	85%
tabcmd/commands/auth
session.py	379	49	49	87%
tabcmd/commands/datasources_and_workbooks
datasources_and_workbooks_command.py	157	18	18	89%
datasources_workbooks_views_url_parser.py	141	10	10	93%
delete_command.py	60	16	16	73%
export_command.py	120	25	25	79%
get_url_command.py	127	47	47	63%
publish_command.py	85	20	20	76%
runschedule_command.py	21	7	7	67%
tabcmd/commands/extracts
create_extracts_command.py	42	8	8	81%
decrypt_extracts_command.py	27	2	2	93%
delete_extracts_command.py	37	6	6	84%
encrypt_extracts_command.py	27	2	2	93%
extracts.py	20	2	2	90%
reencrypt_extracts_command.py	27	2	2	93%
refresh_extracts_command.py	48	10	10	79%
tabcmd/commands/group
create_group_command.py	29	5	5	83%
delete_group_command.py	27	2	2	93%
tabcmd/commands/project
create_project_command.py	46	8	8	83%
delete_project_command.py	35	4	4	89%
publish_samples_command.py	30	4	4	87%
tabcmd/commands/site
create_site_command.py	34	5	5	85%
delete_site_command.py	27	2	2	93%
edit_site_command.py	38	2	2	95%
list_command.py	77	12	12	84%
list_sites_command.py	29	2	2	93%
tabcmd/commands/user
add_users_command.py	27	3	3	89%
create_site_users.py	58	11	11	81%
create_users_command.py	60	10	10	83%
delete_site_users_command.py	43	5	5	88%
user_data.py	221	31	31	86%
tabcmd/execution
_version.py	2	2	2	0%
global_options.py	125	8	8	94%
localize.py	69	11	11	84%
logger_config.py	42	6	6	86%
tabcmd_controller.py	42	7	7	83%
TOTAL	2780	439	84%

Copilot

Pull request overview

This pull request enables mypy's check_untyped_defs configuration to improve type safety across the codebase. The changes focus on adding explicit type annotations, converting static methods to class methods, and improving None-safety checks to satisfy mypy's stricter type checking requirements.

Changes:

Added explicit type annotations throughout the codebase (Optional types, union types, return types)
Converted @staticmethod decorators to @classmethod in command classes, changing logger initialization from __class__.__name__ to cls.__name__
Introduced ParserTest base class for parser tests to provide consistent test infrastructure
Updated Errors.exit_with_error calls to use named exception parameter consistently
Added defensive None checks and assertions to prevent AttributeError in code paths where Optional types are used

Reviewed changes

Copilot reviewed 72 out of 72 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
pyproject.toml	Enabled `check_untyped_defs = true` in mypy configuration
tests/parsers/common_setup.py	Created ParserTest base class with type annotations for test infrastructure
tests/parsers/test_parser_*.py (multiple files)	Updated test classes to inherit from ParserTest instead of unittest.TestCase
tests/parsers/test_parser_add_user.py	Fixed assertion syntax by removing extraneous ", args" expressions
tests/parsers/test_parser_get_url.py	Changed mock_args from dict to list for proper argument parsing
tests/parsers/test_login_parser.py	Added type annotation for commandname variable
tests/commands/test_user_utils.py	Added Optional type hints and None assertions for _parse_line results
tests/commands/test_session.py	Fixed test methods to properly call instance method _allow_prompt; corrected site to site_id
tests/commands/test_run_commands.py	Changed ServerResponseError code parameter from int to string; added missing import
tests/commands/test_projects_utils.py	Removed test case for None input to _parse_project_path_to_list
tests/commands/test_geturl_utils.py	Fixed string escaping in test URL
tests/e2e/tests_integration.py	Moved logger instantiation to module level
tests/e2e/language_tests.py	Added _get_workbook helper method
tabcmd/tabcmd.py	Added defensive None check for traceback to prevent AttributeError
tabcmd/execution/parent_parser.py	Changed parser from class attribute to properly typed instance attribute
tabcmd/commands/auth/session.py	Added Optional type annotations for all session attributes; added None checks with asserts
tabcmd/commands/auth/login_command.py	Converted run_command from static to class method
tabcmd/commands/auth/logout_command.py	Converted run_command from static to class method
tabcmd/commands/server.py	Updated method signature to accept Optional parameters
tabcmd/commands/user/*.py (multiple files)	Converted run_command from static to class methods
tabcmd/commands/user/create_site_users.py	Changed error message handling (incomplete commented code)
tabcmd/commands/user/add_users_command.py	Added exception handling with e.cause logging
tabcmd/commands/site/*.py (multiple files)	Converted run_command methods; updated exit_with_error calls
tabcmd/commands/project/*.py (multiple files)	Converted run_command methods; updated exit_with_error calls
tabcmd/commands/group/*.py (multiple files)	Converted run_command methods; updated exit_with_error calls
tabcmd/commands/extracts/*.py (multiple files)	Converted run_command methods; moved type annotations outside conditionals
tabcmd/commands/datasources_and_workbooks/*.py (multiple files)	Added RequestOptionsType union; converted methods; changed init to pass
tabcmd/commands/datasources_and_workbooks/datasources_workbooks_views_url_parser.py	Added None check for custom_view_item.view

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T06:58:46Z

tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py

    def __init__(self, args):
-        super().__init__(args)
+        pass


The init method now only contains 'pass' and ignores the 'args' parameter. This breaks the parent class contract if Server.init expects these arguments. Since this class inherits from Server and Server doesn't have an init, this change makes the init unnecessary. Consider removing the init method entirely rather than having an empty implementation that accepts but ignores parameters.

Copilot · 2026-02-13T06:58:46Z

tests/commands/test_projects_utils.py

    @staticmethod
    def test_parent_path_to_list():
-        assert Server._parse_project_path_to_list(None) == []
        assert Server._parse_project_path_to_list("") == []


This test case was removed, which appears to test that passing None to _parse_project_path_to_list returns an empty list. This test removal may indicate that None is no longer an acceptable input, but the function implementation still checks for None at line 167 (if project_path is None or project_path == ""). If None is no longer valid input, the None check should be removed from the function. If None is still valid, the test should be retained.

Copilot · 2026-02-13T06:58:47Z

tabcmd/commands/datasources_and_workbooks/publish_command.py

+        connection: TSC.models.ConnectionItem | None = None
        if args.db_username:
+            connection = TSC.models.ConnectionItem()
            connection.connection_credentials = TSC.models.ConnectionCredentials(
                args.db_username, args.db_password, embed=args.save_db_password
            )
        elif args.oauth_username:
+            connection = TSC.models.ConnectionItem()
            connection.connection_credentials = TSC.models.ConnectionCredentials(
                args.oauth_username, None, embed=False, oauth=args.save_oauth
            )
        else:
-            logger.debug("No db-username or oauth-username found in command")
            connection = None
+            logger.debug("No db-username or oauth-username found in command")


The connection variable is now initialized to None and only instantiated within the conditional branches. While this change makes the typing clearer (connection: TSC.models.ConnectionItem | None), it changes the order of operations by moving the logger.debug call after setting connection to None. This reordering moves the "No db-username or oauth-username found" debug message to after the connection = None assignment, which is logically correct but changes the execution flow.

Copilot · 2026-02-13T06:58:47Z

tabcmd/commands/datasources_and_workbooks/export_command.py

            view_example = "/workbook_name/view_name"
            message = "{} [{}]".format(
-                _("export.errors.requires_workbook_view_param").format(__class__.__name__), view_example
+                _("export.errors.requires_workbook_view_param").format("ExportCommand"), view_example


The hardcoded string "ExportCommand" replaces the dynamic class.name reference. While this works correctly after converting from staticmethod to classmethod, it would be more maintainable to use cls.name instead of a hardcoded string. This ensures the class name stays correct if the class is ever renamed or subclassed.

Suggested change

_("export.errors.requires_workbook_view_param").format("ExportCommand"), view_example

_("export.errors.requires_workbook_view_param").format(cls.__name__), view_example

Copilot · 2026-02-13T06:58:47Z

tabcmd/commands/user/add_users_command.py

+        try:
+            UserCommand.act_on_users(logger, server, "added", server.groups.add_user, args)
+        except Exception as e:
+            logger.error(e.__cause__)


The exception handling logs e.cause which may be None for exceptions that don't have an underlying cause. This could result in logging "None" instead of meaningful error information. Consider logging the exception itself (e) or using str(e) to get the exception message, and only log e.cause if it's not None.

Suggested change

logger.error(e.__cause__)

logger.error("Error while adding users: %s", e)

if e.__cause__ is not None:

logger.error("Caused by: %s", e.__cause__)

Copilot · 2026-02-13T06:58:49Z

tests/commands/test_geturl_utils.py


    def test_parse_get_url_to_view_parts_without_slashes(self):
-        url = "views\wb name\\view-name"
+        url = "views\\wb name\\view-name"


The string has an incorrect escape sequence. It should be "views\\wb name\\view-name" (with double backslashes for proper escaping) or a raw string r"views\wb name\view-name". The current form "views\wb name\view-name" will be interpreted with a single backslash which is the intended behavior, but it's inconsistent with typical Python string escaping conventions for test data.

Copilot · 2026-02-13T06:58:49Z

tabcmd/commands/user/create_site_users.py

                    number_of_errors += 1
                    logger.debug(number_of_errors)
-                    error_list.append(e.summary + ": " + e.detail)
+                    error_list.append(e.__class__.__name__) # + ": " + e.__cause__ or "Unknown")


This line appears to be incomplete and contains commented-out code. The comment suggests building an error message with "e.cause" but this is left incomplete with "+ ": " + e.cause or "Unknown")". Either complete the error message construction or remove the comment entirely. Additionally, just appending the class name without any additional context may not provide enough information for debugging.

Copilot · 2026-02-13T06:58:49Z

tests/parsers/common_setup.py

@@ -1,33 +1,43 @@
+import unittest


Module 'unittest' is imported with both 'import' and 'import from'.

Copilot · 2026-02-13T06:58:49Z

tests/parsers/common_setup.py

@@ -1,33 +1,43 @@
+import unittest
+from unittest import mock


Import of 'mock' is not used.

Copilot · 2026-02-13T06:58:50Z

tests/parsers/common_setup.py

 from tabcmd.execution import parent_parser
 from collections import namedtuple
+import argparse
+from typing import Optional, Type, Any


Import of 'Optional' is not used.

jacalata and others added 8 commits August 13, 2025 23:47

typing: from static methods to class methods

63f1ae1

type fixes: session.py

112654e

typing: parser tests

4457b1e

Update test_session.py

4677e5c

Update pyproject.toml

408ce05

fix type errors in /tests

a2a11ae

output with fixes > mypy tests Success: no issues found in 51 source files

Merge branch 'development' into jac/check-untyped-defs

a1f0948

jacalata requested a review from Copilot February 13, 2026 06:52

Copilot started reviewing on behalf of jacalata February 13, 2026 06:52 View session

Refactor version retrieval to use importlib.metadata

3837652

Copilot AI reviewed Feb 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mypy: enable checking untyped code#367

mypy: enable checking untyped code#367
jacalata wants to merge 9 commits intodevelopmentfrom
jac/check-untyped-defs

jacalata commented Oct 8, 2025

Uh oh!

github-actions bot commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	_("export.errors.requires_workbook_view_param").format("ExportCommand"), view_example
	_("export.errors.requires_workbook_view_param").format(cls.__name__), view_example

-            logger.error(e.__cause__)
+            logger.error("Error while adding users: %s", e)
+            if e.__cause__ is not None:
+                logger.error("Caused by: %s", e.__cause__)

Conversation

jacalata commented Oct 8, 2025

Uh oh!

github-actions bot commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant