CI: run stubtest #178
CI: run stubtest #178
Conversation
|
(if this gets merged, all open PRs need to rebase) |
|
Is there any pandas public method that is not in the allow list? I'm not sure I see the value, beyond having a new pandas method or class that we should then add a stub for. We could handle that by just adding things based on the release notes. It seems that we have mismatches on every pandas method and class simply because the pandas source is not fully typed. |
|
It does not create much value right now, for example, all of the functions that still accept positional arguments trigger an error (more or else all functions that have an overload). How about adding stubetst without the whitelist as a standalone test to poe (not part of test_all and test_dist; will not be run on the CI)? |
I don't think we should run it in the CI, but also not sure of the value without the whitelist. |
|
Adding it without the allowlist and not running it on the CI would make it easy to locally run stubtest (for example there are entire files that do not exist in the implementation). Adding stubtest to the CI might be something we can revisit with pandas 2.0 (when many arguments are actually keyword-only). |
Why not run it locally with the allowlist? What are the benefits or disadvantages of using/not using the allowlist when running locally? |
While the allowlist is great to find new regressions (unless the entire API is on the allowlist), the stubtest-generated allowlist has no comments on why a certain function is on the allowlist. I'm mainly interested in running stubtest without an allowlist to see which existing errors can be fixed in pandas-stubs without having to wait for pandas. Could have |
OK, that makes sense.
Seems like we shouldn't add the long allowlist, but then can you also add some documentation about |
| @@ -63,5 +63,8 @@ def run_job(steps: List[Step]) -> None: | |||
| end = time.perf_counter() | |||
| logger.success(f"End: '{step.name}', runtime: {end - start:.3f} seconds.") | |||
|
|
|||
| if not failed: | |||
| __rollback_job(rollback_steps) | |||
There was a problem hiding this comment.
Unrelated: run the rollbacks when none of the steps failed (instead of having to specify the rollback as a step)
scripts/test/__init__.py
Outdated
|
|
||
| def stubtest(allowlist: str): | ||
| stubtest = copy(_step.stubtest) | ||
| stubtest.run = partial(_step.stubtest.run, allowlist=allowlist) |
There was a problem hiding this comment.
a bit ugly but I didn't see another way with the current test structure.
|
Ping me when I should review again. |
Could enable it right now with the enormously long allowlist (
python/mypy#13305 would reduce it a bit but it would still be very long) to at least ensure that new commits stay in sync with the implementation (or are purposefully different from the implementation).