Skip to content

Conversation

@nenb
Copy link

@nenb nenb commented Aug 21, 2025

(WIP, DO NOT MERGE - still waiting for internal review)

Ready for review.

Closes #49

Copy link

@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.

nice, just left a comment

@nenb
Copy link
Author

nenb commented Aug 29, 2025

@Tinche This is almost ready for a final review.

There is just one outstanding error that is arising from the type-checker that I am not certain about and would appreciate your guidance on.

In text.py, readable appears to be defined as both an async and a sync method. The sync method overwrites the async method, and so in practice it is a sync method. (This is my understanding anyway.)

mypy gets a little confused by redefining an async function by a sync function in this way. I can explicitly ignore it, but I first wanted to check with you whether this is deliberate behaviour?

@nenb
Copy link
Author

nenb commented Aug 29, 2025

Oops, I just saw #28! In which case, this PR is ready for review now.

@nenb nenb marked this pull request as ready for review August 29, 2025 17:10
follow_symlinks: bool = True,
loop: AbstractEventLoop | None = ...,
executor: Executor | None = ...,
) -> stat_result: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you possibly separate these "typedefs" with a newline for readability?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, it turns out that the typing style guide recommends to not use blank lines between methods except to group them: https://typing.python.org/en/latest/guides/writing_stubs.html#blank-lines.

I spent a small amount of time trying to override this in ruff but was not successful.

Do you mind very much keeping it the way it is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it turns out that the typing style guide recommends to not use blank lines between methods except to group them: https://typing.python.org/en/latest/guides/writing_stubs.html#blank-lines.

I spent a small amount of time trying to override this in ruff but was not successful.

Do you mind very much keeping it the way it is?

I don't mind, thanks for the reference.


test:
{{ run_prefix }}pytest -x --ff {{ tests_dir }}
{{ run_prefix }}pytest -x --ff {{ tests_dir }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, but optional, you can move these pytest options into the pyproject.toml done like here in case you PR goes first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to 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.

Support for type hints

3 participants