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

[pytx] fix broken gh actions for pytest and lint #1254

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

BarrettOlson
Copy link
Contributor

Summary

Unfortunately the test breaking were not showing up locally, so I used a dev container to get a close as possible to the gh action ENV so I could repro and fix.

Test Plan

py.test and gh actions

@github-actions github-actions bot added the python-threatexchange Items related to the threatexchange python tool / library label Jan 19, 2023
@BarrettOlson BarrettOlson marked this pull request as ready for review January 19, 2023 21:20
@@ -272,7 +272,7 @@ class SignalExchangeAPIWithSimpleUpdates(
state.TFetchCheckpoint,
state.TFetchedSignalMetadata,
t.Tuple[str, str],
state.TFetchedSignalMetadata,
t.Any,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dcallies I'm not sure on the typing here. All changes besides t.Any expanded to 10 more errors where SignalExchangeAPIWithSimpleUpdates is being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is definitely not right here, the other option is to just # type: ignore for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't get mypy to understand the # type: ignore directive here. No matter where I put it (above, same line, below as part of class) it get a version of the following:

threatexchange/exchanges/signal_exchange_api.py:269: error: Variance of TypeVar "state.TFetchedSignalMetadata" incompatible with variance in parent type  [type-var]
threatexchange/exchanges/signal_exchange_api.py:277: error: Unused "type: ignore" comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synced offline, as ignore was not working I will merge as t.Any but created high-pri issue #1255

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

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

blocking: That Any change is a bit on the sketch end, and its on one of the key APIs.

If ignoring it will get the typechecker okay, I can come back to it in the future, which I like better than swapping to Any

@@ -72,7 +72,7 @@ def assert_cli_output(
assert lines[line] == expected_line_output

def assert_cli_usage_error(
self, args: t.Iterable[str], msg_regex: str = None
self, args: t.Iterable[str], msg_regex: str = ""
) -> None:
with pytest.raises((CommandError, E2ETestSystemExit), match=msg_regex) as ex:
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -272,7 +272,7 @@ class SignalExchangeAPIWithSimpleUpdates(
state.TFetchCheckpoint,
state.TFetchedSignalMetadata,
t.Tuple[str, str],
state.TFetchedSignalMetadata,
t.Any,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is definitely not right here, the other option is to just # type: ignore for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed python-threatexchange Items related to the threatexchange python tool / library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants