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

[HMA] Removing IContentTypeConfigStore #1745

Merged
merged 7 commits into from
Feb 3, 2025

Conversation

b8zhong
Copy link
Contributor

@b8zhong b8zhong commented Feb 2, 2025

Summary

Removes IContentTypeConfigStore, as requested in the last part of #1688.

Note: I plan to add copyright boilerplate to the currently empty init + bump minor version in a separate PR like you requested here! #1707

Test Plan

N/A

@b8zhong
Copy link
Contributor Author

b8zhong commented Feb 2, 2025

CI fails are expected here I believe..

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.

Hmm, CI fails are not expected here, and will predict HMA library failures. We need a bumped version which contains the init fix.

This is the same problem that the MLH folks were running into, which you already found the solution to - the missing __init__.py file.

To fix, you'll need to break this up a sequence of PRs:

1. Add __init__.py to storage
2. Version bump
3. Now can remove.

EDIT: Now I'm confused, the storage interface is not in the right place. It's currently at https://github.com/facebook/ThreatExchange/tree/main/python-threatexchange/threatexchange/cli/storage but it should be in https://github.com/facebook/ThreatExchange/tree/main/python-threatexchange/threatexchange/storage

This looks like a mistake I made in review -_-

@@ -23,6 +23,7 @@
import flask
from threatexchange.cli.storage.interfaces import ISignalTypeConfigStore
Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh, this should have been threatexchange.storage.interfaces

@b8zhong
Copy link
Contributor Author

b8zhong commented Feb 3, 2025

Hey! I'm around now; do you still want to take over?

(Actually I just saw your edit, good catch I did not realize that lol)

@b8zhong
Copy link
Contributor Author

b8zhong commented Feb 3, 2025

(Closing as addressed in #1747 & #1748

@b8zhong b8zhong closed this Feb 3, 2025
@Dcallies
Copy link
Contributor

Dcallies commented Feb 3, 2025

@b8zhong - didn't quite address! This PR is still needed, I just fixed the underlying stuff for you!

@Dcallies Dcallies reopened this Feb 3, 2025
@b8zhong
Copy link
Contributor Author

b8zhong commented Feb 3, 2025

There we go, mypy is finally happy

@Dcallies
Copy link
Contributor

Dcallies commented Feb 3, 2025

Nice!

@Dcallies Dcallies merged commit e86ff4d into facebook:main Feb 3, 2025
5 checks passed
@b8zhong b8zhong deleted the IContentTypeConfigStore branch February 3, 2025 14:19
@b8zhong
Copy link
Contributor Author

b8zhong commented Feb 3, 2025

Thanks for merging as always!

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.

3 participants