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

Fix all fixable stubtest_allowlist entries in SQLAlchemy #9596

Merged
merged 30 commits into from
Apr 14, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Jan 27, 2023

This PR focuses solely on fixing as many stubtest_allowlist entries as possible (unless they're caused by an issue with subtest itself).
Also regrouped and reorganized some entries while updating comments.

@github-actions

This comment has been minimized.

@Avasam Avasam changed the title Fix most stubtest_allowlist entries in SQLAlchemy Fix all fixable stubtest_allowlist entries in SQLAlchemy Jan 27, 2023
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you! Overall this looks great, just a few comments below:

Comment on lines 65 to 69
class loader_option(Generic[_F]):
name: str
_dynamic: _F
fn: _F
__call__: _F # Cheesy "__call__" definition to use the dynamic methods instead
Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to add a couple of test cases for this, so we can be sure the type hints are working correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the recent changes are much simpler and probably won't require test cases. I'll let you decide.

Copy link
Member

Choose a reason for hiding this comment

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

I'd still prefer one or two test cases, if that's okay :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(tests added in a previous commit)

Comment on lines 37 to 42
class register(Generic[_F]):
fns: dict[str, _F]
@classmethod
def init(cls: type[Self], fn: _F) -> Self: ...
def for_db(self: Self, *dbnames: str) -> Callable[[_F], Self]: ...
__call__: _F
Copy link
Member

Choose a reason for hiding this comment

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

Same here -- maybe add a test case or two?

Copy link
Collaborator Author

@Avasam Avasam Jan 30, 2023

Choose a reason for hiding this comment

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

It looks like the tests I can do for register are really limited. For example, because it's generic, I get [assert-type] errors like:
Expression is of type "register[Callable[[object, object, object], None]]", not "register[Any]", or
Expression is of type "register[Callable[[object, object, object], None]]", not "register[Callable[[object, object, object], None]]"

At least I can test that the first parameter is correctly replaced.

https://github.com/python/typeshed/actions/runs/4045446150/jobs/6956947952

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(tests added in a previous commit)

@hauntsaninja
Copy link
Collaborator

(not a review, but those line diff counts look great!!)

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 29, 2023

(not a review, but those line diff counts look great!!)

One of the few times we like seeing a lot of red ^^


I've yet to add some tests. But that's all the time I had right now. Will tackle those later.

@github-actions

This comment has been minimized.

@Avasam Avasam requested a review from AlexWaygood January 30, 2023 12:11
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 30, 2023

#9622 #9650 concerning "Check file consistency"

@github-actions

This comment has been minimized.

pass


no_args(cfg="")
Copy link
Member

Choose a reason for hiding this comment

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

Is it useful to assert_type the return type of these and the next functions?

Copy link
Collaborator Author

@Avasam Avasam Mar 22, 2023

Choose a reason for hiding this comment

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

Idk if it's useful because of:
https://github.com/python/typeshed/pull/9596/files#diff-548edbccead15d5b517e8f12d71ae94f90c0e5a7dbdb8eea1657139fe8d74a92R31-R32

# Impossible to specify the args from the generic Callable in the current type system
def __call__(self, cfg: str | URL | _ConfigProtocol, *arg: Any) -> str | URL | None: ...  # AnyOf[str | URL | None]

In theory the return type should be infered from the undecorated function (which would be worth testing). But we're unable to (or at least I was at the moment of writing this stub).

@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Mar 16, 2023

(I've been quite occupied last week, and I got sick this week, will go through updated reviews soon once I feel a bit better)

@github-actions

This comment has been minimized.

@Avasam Avasam requested review from JelleZijlstra and AlexWaygood and removed request for JelleZijlstra March 22, 2023 17:11
@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Mar 31, 2023

@AlexWaygood @JelleZijlstra Can I get an updated review on this? I answered the comments and I don't think there was anything new to add (just merged main to resolve conflicts)

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/server/schemas/sorting.py:38: error: "coalesce" has no attribute "asc"  [attr-defined]
- src/prefect/server/schemas/sorting.py:41: error: "coalesce" has no attribute "desc"  [attr-defined]

@JelleZijlstra JelleZijlstra self-requested a review March 31, 2023 19:41
@JelleZijlstra
Copy link
Member

@AlexWaygood let's merge this?

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 14, 2023

@AlexWaygood let's merge this?

I remember having some qualms a while back about a few minor things that I wanted to investigate further, but I can't remember what they are anymore, and I don't think they really matter in the scheme of things :)

Let's go for it.

@AlexWaygood
Copy link
Member

@Avasam sorry for the delay on this one 😔

@JelleZijlstra JelleZijlstra merged commit b0ed50e into python:main Apr 14, 2023
@Avasam Avasam deleted the sqlalchemy-stubtest branch April 14, 2023 23:16
@Avasam
Copy link
Collaborator Author

Avasam commented Apr 15, 2023

I remember having some qualms a while back about a few minor things that I wanted to investigate further, but I can't remember what they are anymore, and I don't think they really matter in the scheme of things :)

There's a follow-up PR anyway (which may or may not have conflicts now, I haven't checked). If you remember anything you can mention it there too.

@Avasam
Copy link
Collaborator Author

Avasam commented Apr 29, 2023

Just leaving a comment here to link to #9127 (comment)

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.

4 participants