-
Notifications
You must be signed in to change notification settings - Fork 709
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 mypy pep561 #1088
Fix mypy pep561 #1088
Conversation
Signed-off-by: Kang Wenjing <[email protected]>
@ashwinvaidya17 Could you please help double check this? Thanks! |
Do we really need |
Without this pr, even pure |
From this link:
|
@@ -107,6 +107,7 @@ def get_required_packages(requirement_files: list[str]) -> list[str]: | |||
packages=find_packages(where="src", include=["anomalib", "anomalib.*"]), | |||
install_requires=INSTALL_REQUIRES, | |||
extras_require=EXTRAS_REQUIRE, | |||
include_package_data=True, | |||
package_data={"": ["config.yaml"]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I tested
package_data={"": ["config.yaml"], "anomalib": ["py.typed"]},
should be sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've tested it as well. It worked. But if we are going to use MANIFEST.in
, we need to use the include_package_data=True.
If we are going to use package_data={"": ["config.yaml"], "anomalib": ["py.typed"]},
, then we need to remove py.typed
from MANIFEST.in
, because they have duplicated functionality, which is to include the same file.
To sum up, if we do not use include_package_data=True, MANIFEST.in
will not take effect.
Description
With this PR, when running
python setup.py install
,python setup.py install sdist
orpython setup.py sdist bdist_wheel
,py.typed
will be included in the build package.Changes
Checklist