-
Notifications
You must be signed in to change notification settings - Fork 564
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: Issue #2307 #2439
base: master
Are you sure you want to change the base?
Fix: Issue #2307 #2439
Conversation
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.
Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased)
section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed
CHANGELOG updated or no update needed, thanks! 😄
|
we'll remove py3.8 in the next couple days and then we can retry the CI runs. thanks @harshit-wadhwani ! |
mypy fails, could you take another look, please, @harshit-wadhwani?
|
those mypy errors are outside of the code that @harshit-wadhwani has touched, though i agree it would be great if they can fix the errors. it seems to be due to stricter checking that mypy can now do: mypy now knows that only some strings can be put into the to initialize this list, we could use the list of features in VALID_FEATURE_NAMES = set()
for features in capa.rules.SUPPORTED_FEATURES.values():
for feature in features:
VALID_FEATURE_NAMES.add(feature.name)
...
assert type_name in VALID_FEATURE_NAMES hm, but this won't satisfy mypy, cause mypy can't look into this list. so instead i think we need to hardcode the list, like: VALID_FEATURE_NAMES = ("os", "arch", ...)
...
assert type_name in VALID_FEATURE_NAMES the downside is that we need to maintain this list, keeping it up to date whenever we add a new feature. but i dont think that will be common or require much work. |
I will take a look into those errors. |
@harshit-wadhwani have you had a chance to take a look here, yet? |
Resolve type incompatibility by introducing FeatureType type alias with strict literal types
I ran pre-commit with dev container and it passes everything, but not on the github actions, any suggestions ? |
Checklist