Skip to content

chore(typing): fix/ignore utils, tpch warnings#2426

Merged
MarcoGorelli merged 12 commits intomainfrom
fix-utils-warnings
Apr 26, 2025
Merged

chore(typing): fix/ignore utils, tpch warnings#2426
MarcoGorelli merged 12 commits intomainfrom
fix-utils-warnings

Conversation

@dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Apr 23, 2025

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

image

`pyright` did a good job spotting that one
Fixes 2/6 errors and shrinks the script
These have been popping up and bugging me for 2 weeks 🙈
#2351 (comment)
These are all the same issue, can be fixed with a constrained `TypeVar`
@dangotbanned
Copy link
Member Author

dangotbanned commented Apr 23, 2025

Hmmm, not sure what's going on in ci 🤔
https://results.pre-commit.ci/run/github/760058710/1745426178.-zxDJo8RTMqkvd3uYQp_eg

This is working for me locally:

from __future__ import annotations

from typing import Any
from typing import Iterator

import narwhals as nw


def _is_public_method_or_property(obj: Any) -> bool:
    return (
        (inspect.isfunction(obj) or isinstance(obj, property))
        and not obj.__name__[0].isupper()
        and obj.__name__[0] != "_"
    )


def iter_api_reference_names(tp: type[Any]) -> Iterator[str]:
    for name, _ in inspect.getmembers(tp, _is_public_method_or_property):
        yield name


it = iter_api_reference_names(nw.DataFrame)
>>> list(it)
Output

['clone',
 'collect_schema',
 'columns',
 'drop',
 'drop_nulls',
 'estimated_size',
 'explode',
 'filter',
 'gather_every',
 'get_column',
 'group_by',
 'head',
 'implementation',
 'is_duplicated',
 'is_empty',
 'is_unique',
 'item',
 'iter_columns',
 'iter_rows',
 'join',
 'join_asof',
 'lazy',
 'null_count',
 'pipe',
 'pivot',
 'rename',
 'row',
 'rows',
 'sample',
 'schema',
 'select',
 'shape',
 'sort',
 'tail',
 'to_arrow',
 'to_dict',
 'to_native',
 'to_numpy',
 'to_pandas',
 'to_polars',
 'unique',
 'unpivot',
 'with_columns',
 'with_row_index',
 'write_csv',
 'write_parquet']

Update 1

Ah there's the issue https://github.com/python/typeshed/blob/ae57d69c9354147bcf7086d6db47af8f8c82bf7b/stdlib/builtins.pyi#L1266-L1272

class property:
    fget: Callable[[Any], Any] | None
    fset: Callable[[Any, Any], None] | None
    fdel: Callable[[Any], None] | None
    __isabstractmethod__: bool
    if sys.version_info >= (3, 13):
        __name__: str

Update 2

Fixed in (86dda9b) 🥳

@dangotbanned dangotbanned marked this pull request as ready for review April 23, 2025 17:58
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks - should we also run mypy on these folders in ci if the types are fixed?

from narwhals import DataFrame
from narwhals import LazyFrame

FrameT = TypeVar("FrameT", DataFrame[Any], LazyFrame[Any])
Copy link
Member

Choose a reason for hiding this comment

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

what's the issue with importing FrameT from narwhals._typing?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dangotbanned
Copy link
Member Author

thanks - should we also run mypy on these folders in ci if the types are fixed?

Thanks @MarcoGorelli

I suppose we could do!
I'm not sure if mypy will pass, my goal was really just to be able to open these files in VSCode and not be bombarded by warnings 😄

Generally, I think it would be a positive step forwards to enable ruff, mypy, pyright on all of these directories.
If there are particular rules/codes that are frustrating - we have options to disable things in a granular way 🙂

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks!

@MarcoGorelli
Copy link
Member

separately, this makes me wonder if we need to redefine FrameT 🤔

@MarcoGorelli MarcoGorelli merged commit 41475ad into main Apr 26, 2025
29 checks passed
@MarcoGorelli MarcoGorelli deleted the fix-utils-warnings branch April 26, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants