Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional type hints added to stub openpyxl #9487

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 39 additions & 33 deletions stubs/openpyxl/openpyxl/cell/cell.pyi
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
from _typeshed import Incomplete
from datetime import datetime
from typing import Any

from openpyxl.comments.comments import Comment
from openpyxl.styles.cell_style import StyleArray
from openpyxl.styles.styleable import StyleableObject
from openpyxl.worksheet.hyperlink import Hyperlink
from openpyxl.worksheet.worksheet import Worksheet

__docformat__: str
TIME_TYPES: Incomplete
Expand All @@ -19,60 +25,60 @@ TYPE_ERROR: str
TYPE_FORMULA_CACHE_STRING: str
VALID_TYPES: Incomplete

def get_type(t, value): ...
def get_time_format(t): ...
def get_type(t: Any, value: Any) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

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

  • t looks like it has to be a class of some kind, due to this line here, where it's used as a key in a dictionary that only has classes as keys: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L73
  • value could be literally any object, so it's better to use object here rather than Any. Any is an "escape hatch" in the type system for things that are truly inexpressible. It's very unsafe, so it's better to use object whenever possible.
  • The function always either returns a str or None.
Suggested change
def get_type(t: Any, value: Any) -> Any: ...
def get_type(t: type, value: object) -> str | None: ...

def get_time_format(t: Any) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_time_format(t: Any) -> Any: ...
def get_time_format(t: datetime) -> str: ...


class Cell(StyleableObject):
row: Incomplete
column: Incomplete
row: int
column: int
data_type: str
def __init__(
self,
worksheet,
row: Incomplete | None = ...,
column: Incomplete | None = ...,
value: Incomplete | None = ...,
style_array: Incomplete | None = ...,
worksheet: Worksheet,
row: int | None = ...,
column: int | None = ...,
value: str | float | int | datetime | None = ...,
Copy link
Member

Choose a reason for hiding this comment

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

PEP 484 specifies that functions annotated as accepting float should also accept int, so int is redundant in this union

Suggested change
value: str | float | int | datetime | None = ...,
value: str | float | datetime | None = ...,

style_array: StyleArray | None = ...,
) -> None: ...
@property
def coordinate(self): ...
def coordinate(self) -> str: ...
@property
def col_idx(self): ...
def col_idx(self) -> int: ...
@property
def column_letter(self): ...
def column_letter(self) -> str: ...
@property
def encoding(self): ...
def encoding(self) -> str: ...
@property
def base_date(self): ...
def check_string(self, value): ...
def check_error(self, value): ...
def base_date(self) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

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

This property is a datetime:

>>> from openpyxl import Workbook
>>> cell = Workbook().active['A1']
>>> type(cell.base_date)
<class 'datetime.datetime'>
Suggested change
def base_date(self) -> Any: ...
def base_date(self) -> datetime: ...

def check_string(self, value: str): ...
def check_error(self, value: Any) -> Any | str: ...
Copy link
Member

Choose a reason for hiding this comment

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

  • This will accept literally any object, so object is better than Any for the input type
  • As far as I can tell, it always returns str
Suggested change
def check_error(self, value: Any) -> Any | str: ...
def check_error(self, value: object) -> str: ...

@property
def value(self): ...
def value(self) -> str | float | int | datetime | None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def value(self) -> str | float | int | datetime | None: ...
def value(self) -> str | float | datetime | None: ...

@value.setter
def value(self, value) -> None: ...
def value(self, value: str | float | int | datetime | None) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def value(self, value: str | float | int | datetime | None) -> None: ...
def value(self, value: str | float | datetime | None) -> None: ...

@property
def internal_value(self): ...
def internal_value(self) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this just have the same type as the value property? https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L203-220

@property
def hyperlink(self): ...
def hyperlink(self) -> Hyperlink | str: ...
Copy link
Member

Choose a reason for hiding this comment

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

The hyperlink property just returns whatever self._hyperlink is set to. And self._hyperlink can be None: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L236. That means that this property can also return None.

Moreover, if you do cell.hyperlink = 'https://www.bbc.co.uk' or whatever, that string is implicitly converted to become a Hyperlink object before it is stored in the self._hyperlink attribute: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L238-239. That means that this property never returns a str:

Suggested change
def hyperlink(self) -> Hyperlink | str: ...
def hyperlink(self) -> Hyperlink | None: ...

@hyperlink.setter
def hyperlink(self, val) -> None: ...
def hyperlink(self, val: Hyperlink | str | None) -> None: ...
@property
def is_date(self): ...
def offset(self, row: int = ..., column: int = ...): ...
def is_date(self) -> bool: ...
def offset(self, row: int = ..., column: int = ...) -> Cell: ...
@property
def comment(self): ...
def comment(self) -> Comment: ...
Copy link
Member

Choose a reason for hiding this comment

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

This can also be None: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L292-294

Suggested change
def comment(self) -> Comment: ...
def comment(self) -> Comment | None: ...

@comment.setter
def comment(self, value) -> None: ...
def comment(self, value: Comment) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

It's okay to pass None in as well: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L292-294

Suggested change
def comment(self, value: Comment) -> None: ...
def comment(self, value: Comment | None) -> None: ...


class MergedCell(StyleableObject):
data_type: str
comment: Incomplete
hyperlink: Incomplete
row: Incomplete
column: Incomplete
def __init__(self, worksheet, row: Incomplete | None = ..., column: Incomplete | None = ...) -> None: ...
comment: Comment
hyperlink: Hyperlink
Comment on lines +75 to +76
Copy link
Member

Choose a reason for hiding this comment

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

Both of these attributes have None as a default value: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L310

Suggested change
comment: Comment
hyperlink: Hyperlink
comment: Comment | None
hyperlink: Hyperlink | None

row: int
column: int
def __init__(self, worksheet: Worksheet, row: int | None = ..., column: int | None = ...) -> None: ...
@property
def coordinate(self): ...
value: Incomplete
def coordinate(self) -> str: ...
value: str | float | int | datetime | None

def WriteOnlyCell(ws: Incomplete | None = ..., value: Incomplete | None = ...): ...
def WriteOnlyCell(ws: Worksheet | None = ..., value: str | float | int | datetime | None = ...) -> Cell: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def WriteOnlyCell(ws: Worksheet | None = ..., value: str | float | int | datetime | None = ...) -> Cell: ...
def WriteOnlyCell(ws: Worksheet | None = ..., value: str | float | datetime | None = ...) -> Cell: ...

12 changes: 9 additions & 3 deletions stubs/openpyxl/openpyxl/reader/excel.pyi
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
from _typeshed import Incomplete
from typing import Any

from openpyxl.chartsheet.chartsheet import Chartsheet
from openpyxl.workbook.workbook import Workbook

SUPPORTED_FORMATS: Incomplete

Expand All @@ -10,7 +14,7 @@ class ExcelReader:
data_only: Incomplete
keep_links: Incomplete
shared_strings: Incomplete
def __init__(self, fn, read_only: bool = ..., keep_vba=..., data_only: bool = ..., keep_links: bool = ...) -> None: ...
def __init__(self, fn: Any, read_only: bool = ..., keep_vba=..., data_only: bool = ..., keep_links: bool = ...) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Any is a bad type here. If we look at the source code, we can see that fn is passed to _validate_archive() here: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/reader/excel.py#L124

_validate_archive() accepts file-like objects that have a read() method which returns bytes, or strings. For anything else, it will throw an exception: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/reader/excel.py#L76-78.

We have a protocol in the _typeshed directory, SupportsRead, which is good for representing "any object that has a read() method". I recommend you use it here (you'll need to add from _typeshed import SupportsRead at the top of the file).

Suggested change
def __init__(self, fn: Any, read_only: bool = ..., keep_vba=..., data_only: bool = ..., keep_links: bool = ...) -> None: ...
def __init__(self, fn: SupportsRead[bytes] | str, read_only: bool = ..., keep_vba=..., data_only: bool = ..., keep_links: bool = ...) -> None: ...

package: Incomplete
def read_manifest(self) -> None: ...
def read_strings(self) -> None: ...
Expand All @@ -19,8 +23,10 @@ class ExcelReader:
def read_workbook(self) -> None: ...
def read_properties(self) -> None: ...
def read_theme(self) -> None: ...
def read_chartsheet(self, sheet, rel) -> None: ...
def read_chartsheet(self, sheet: Chartsheet, rel: Any) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure what type rel is meant to have, but I'm pretty sure you can't just pass any object in here. If we don't know the type, we'd prefer to leave it unannotated:

Suggested change
def read_chartsheet(self, sheet: Chartsheet, rel: Any) -> None: ...
def read_chartsheet(self, sheet: Chartsheet, rel) -> None: ...

Copy link
Contributor

Choose a reason for hiding this comment

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

rel is a Relationship from openpyxl.packaging.relationship

def read_worksheets(self) -> None: ...
def read(self) -> None: ...

def load_workbook(filename, read_only: bool = ..., keep_vba=..., data_only: bool = ..., keep_links: bool = ...): ...
def load_workbook(
filename: str, read_only: bool = ..., keep_vba: bool = ..., data_only: bool = ..., keep_links: bool = ...
Copy link
Member

Choose a reason for hiding this comment

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

As with the fn argument to ExcelReader.__init__, the filename parameter here can also be any object that has a read() method that returns bytes. _typeshed.SupportsRead would be useful here:

Suggested change
filename: str, read_only: bool = ..., keep_vba: bool = ..., data_only: bool = ..., keep_links: bool = ...
filename: SupportsRead[bytes] | str, read_only: bool = ..., keep_vba: bool = ..., data_only: bool = ..., keep_links: bool = ...

) -> Workbook: ...
75 changes: 40 additions & 35 deletions stubs/openpyxl/openpyxl/workbook/workbook.pyi
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
from _typeshed import Incomplete
from typing import Any, Iterator
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to use PEP 585 imports wherever possible

Suggested change
from typing import Any, Iterator
from collections.abc import Iterator
from typing import Any


from openpyxl.chartsheet.chartsheet import Chartsheet
from openpyxl.styles.named_styles import NamedStyle
from openpyxl.worksheet.worksheet import Worksheet

INTEGER_TYPES: Incomplete

Expand All @@ -20,54 +25,54 @@ class Workbook:
views: Incomplete
def __init__(self, write_only: bool = ..., iso_dates: bool = ...) -> None: ...
@property
def epoch(self): ...
def epoch(self) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

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

This is always a datetime.datetime object: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L119-123

Suggested change
def epoch(self) -> Any: ...
def epoch(self) -> datetime: ...

@epoch.setter
def epoch(self, value) -> None: ...
def epoch(self, value: Any) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

This raises ValueError if you try to set it to a non-datetime object: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L126-130

Suggested change
def epoch(self, value: Any) -> None: ...
def epoch(self, value: datetime) -> None: ...

@property
def read_only(self): ...
def read_only(self) -> bool: ...
@property
def data_only(self): ...
def data_only(self) -> bool: ...
@property
def write_only(self): ...
def write_only(self) -> bool: ...
@property
def excel_base_date(self): ...
def excel_base_date(self) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def excel_base_date(self) -> Any: ...
def excel_base_date(self) -> datetime: ...

@property
def active(self): ...
def active(self) -> Worksheet: ...
Copy link
Member

Choose a reason for hiding this comment

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

This can also be None (the except IndexError branch here: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L150-159)

Suggested change
def active(self) -> Worksheet: ...
def active(self) -> Worksheet | None: ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with None.
It is any sheet class belonging to _WorkbookChild actually: Worksheet, WriteOnlyWorksheet and ChartSheet

Copy link
Member

Choose a reason for hiding this comment

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

The docstring indicates that it's a Worksheet, but I'll trust your judgement here!

https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L154

@active.setter
def active(self, value) -> None: ...
def create_sheet(self, title: Incomplete | None = ..., index: Incomplete | None = ...): ...
def move_sheet(self, sheet, offset: int = ...) -> None: ...
def remove(self, worksheet) -> None: ...
def remove_sheet(self, worksheet) -> None: ...
def create_chartsheet(self, title: Incomplete | None = ..., index: Incomplete | None = ...): ...
def get_sheet_by_name(self, name): ...
def __contains__(self, key): ...
def index(self, worksheet): ...
def get_index(self, worksheet): ...
def __getitem__(self, key): ...
def __delitem__(self, key) -> None: ...
def __iter__(self): ...
def get_sheet_names(self): ...
def active(self, value: Worksheet) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

int is also okay here: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L166-168

Suggested change
def active(self, value: Worksheet) -> None: ...
def active(self, value: Worksheet | int) -> None: ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with Int.
It is any sheet class belonging to _WorkbookChild actually: Worksheet, WriteOnlyWorksheet and ChartSheet

def create_sheet(self, title: str | None = ..., index: int | None = ...): ...
def move_sheet(self, sheet: Worksheet, offset: int = ...) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

This can also be str: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L222-223

Suggested change
def move_sheet(self, sheet: Worksheet, offset: int = ...) -> None: ...
def move_sheet(self, sheet: Worksheet | str, offset: int = ...) -> None: ...

def remove(self, worksheet: Worksheet) -> None: ...
def remove_sheet(self, worksheet: Worksheet) -> None: ...
def create_chartsheet(self, title: str | None = ..., index: int | None = ...) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't return None, it returns a new Chartsheet object: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L248-251

Suggested change
def create_chartsheet(self, title: str | None = ..., index: int | None = ...) -> None: ...
def create_chartsheet(self, title: str | None = ..., index: int | None = ...) -> Chartsheet: ...

def get_sheet_by_name(self, name: str) -> Worksheet: ...
def __contains__(self, key: str) -> bool: ...
def index(self, worksheet: Worksheet) -> int: ...
def get_index(self, worksheet: Worksheet) -> int: ...
def __getitem__(self, key: str) -> Worksheet: ...
def __delitem__(self, key: str) -> None: ...
def __iter__(self) -> Iterator[Worksheet]: ...
def get_sheet_names(self) -> list[Worksheet]: ...
@property
def worksheets(self): ...
def worksheets(self) -> list[Worksheet]: ...
@property
def chartsheets(self): ...
def chartsheets(self) -> list[Chartsheet]: ...
@property
def sheetnames(self): ...
def sheetnames(self) -> list[str]: ...
def create_named_range(
self, name, worksheet: Incomplete | None = ..., value: Incomplete | None = ..., scope: Incomplete | None = ...
self, name: str, worksheet: Worksheet | None = ..., value: str | Any | None = ..., scope: Incomplete | None = ...
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to continue to use Incomplete here

Suggested change
self, name: str, worksheet: Worksheet | None = ..., value: str | Any | None = ..., scope: Incomplete | None = ...
self, name: str, worksheet: Worksheet | None = ..., value: str | Incomplete | None = ..., scope: Incomplete | None = ...

) -> None: ...
def add_named_style(self, style) -> None: ...
def add_named_style(self, style: Any) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

This calls self._named_styles.append(): https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L344. self._named_styles is a NamedStyleList object, and NamedStyleList.append() raises TypeError if you pass it an object that isn't a NamedStyle: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/styles/named_styles.py#L190-191

Suggested change
def add_named_style(self, style: Any) -> None: ...
def add_named_style(self, style: NamedStyle) -> None: ...

@property
def named_styles(self): ...
def get_named_ranges(self): ...
def add_named_range(self, named_range) -> None: ...
def get_named_range(self, name): ...
def remove_named_range(self, named_range) -> None: ...
def named_styles(self) -> list[NamedStyle]: ...
Copy link
Member

Choose a reason for hiding this comment

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

This returns a list of strs, not a list of NamedStyles

Suggested change
def named_styles(self) -> list[NamedStyle]: ...
def named_styles(self) -> list[str]: ...

def get_named_ranges(self) -> list[Any]: ...
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't return a list, it returns whatever self.defined_names.definedName` is: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L359

self.defined_names is a DefinedNameList object: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L65

The definedName attribute of DefinedNameList objects is not a list: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/defined_name.py#L171-175

def add_named_range(self, named_range: Any) -> None: ...
def get_named_range(self, name: Any) -> Any: ...
def remove_named_range(self, named_range: Any) -> None: ...
Comment on lines +69 to +71
Copy link
Member

Choose a reason for hiding this comment

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

Leave these unannotated if you don't know what the type is. In typeshed, we use Any to mean "we know what the type is, but we can't express it in the type system".

@property
def mime_type(self): ...
def save(self, filename) -> None: ...
def mime_type(self) -> Any: ...
Copy link
Member

Choose a reason for hiding this comment

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

Leave this unannotated if you don't know what the type is

def save(self, filename: str) -> None: ...
@property
def style_names(self): ...
def copy_worksheet(self, from_worksheet): ...
def style_names(self) -> list[NamedStyle]: ...
Copy link
Member

Choose a reason for hiding this comment

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

This returns a list of strs, not a list of NamedStyles

Suggested change
def style_names(self) -> list[NamedStyle]: ...
def style_names(self) -> list[str]: ...

def copy_worksheet(self, from_worksheet: Worksheet) -> Worksheet: ...
Copy link
Member

Choose a reason for hiding this comment

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

This returns a WorksheetCopy object, not a Worksheet object: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/workbook.py#L433-435

Suggested change
def copy_worksheet(self, from_worksheet: Worksheet) -> Worksheet: ...
def copy_worksheet(self, from_worksheet: Worksheet) -> WorksheetCopy: ...

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't return WorksheetCopy, but rather a Worksheet or WriteOnlyWorksheet.

The WorksheetCopy operatates on the result of Workbook.create_sheet(), but doesn't itself get returned.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

def close(self) -> None: ...
Loading