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

rules: use Scope enum instead of constants #1764

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

williballenthin
Copy link
Collaborator

this PR updates code across capa to prefer to use Scope.FOO instead of FOO_SCOPE. before, we had two ways of doing things. now we have one. preferring to use the enum is better because it lets type checkers reason about the set of values, rather than arbitrary strings.

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

@williballenthin williballenthin added enhancement New feature or request breaking-change introduces a breaking change that should be released in a major version labels Aug 25, 2023
@williballenthin
Copy link
Collaborator Author

we should review the rest of the capa code base for similar constructs that we can simplify.

Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

edit: well, tests disagree... :)

@williballenthin
Copy link
Collaborator Author

i think the test failures are in the base branch, too

Copy link
Collaborator

@yelhamer yelhamer left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

Do you think there would be any confusion/mix-up regarding the Scopes and Scope classes? Should we rename Scopes to maybe RuleScopes?

As for the tests, they are failing due to some result document adjustments I believe... I'll work on that and try to get them green again.

@williballenthin
Copy link
Collaborator Author

i could see that someone might temporarily mix up Scope and Scopes, but mypy should be able to highlight the differences pretty quickly, since they have different shapes. i don't have a strong feeling either way.

@yelhamer
Copy link
Collaborator

I see. Let's stick to this (what you proposed in this PR) instead then. I'll merge.

@yelhamer yelhamer merged commit d5daa79 into dynamic-feature-extraction Aug 25, 2023
5 of 15 checks passed
@yelhamer yelhamer deleted the fix/scope-enum-usage branch August 25, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change introduces a breaking change that should be released in a major version enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants