-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update getting started documentation and dependencies #202
Conversation
Includes mypy and coverage
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.
LGTM! Just a couple of nitpicks to make it more Pythonic.
# Finding the longest common substring of a list of strings, by repeatedly | ||
# calling SequenceMatcher's find_longest_match. The 1st candidate is cached | ||
# in the 2nd sequence (b), then following candidates are set as the 1st | ||
# sequence (a). The longest match between a and b is then found, each time | ||
# reducing the search region of b based on the previous match. | ||
# See: https://docs.python.org/3/library/difflib.html#difflib.SequenceMatcher | ||
for cand in candidates: | ||
if matcher is None: | ||
if cand is None: |
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.
If we are checking for None
only, your code works, but if we want to check for empty list, False, and any other zero value, this is the way to do that.
if cand is None: | |
if not cand: |
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.
If we don't trust mypy's type verification, why use it? candidates
is supposed to be a List[Optional[LogQLLineFilterInfo]]
- so yes, it should only be None
or a valid object.
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.
Mypy is not perfect and has misses.
if matcher is None: | ||
if cand is None: | ||
return None # prior check should have caught this, but double checking | ||
if matcher is None or value is None: |
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.
Same as above.
if matcher is None or value is None: | |
if not matcher or not value: |
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.
The matcher
and value
objects are explicitly set to None
in the previous lines (562-563) - this is intended to catch those. The addition of value is None
was simply to stop irrelevant mypy type errors.
#201 reported that our getting started documentation still suggested installed our outdated mirror for sigma-cli, and likely led to issues with the version of pySigma installed. This PR updates that documentation to reflect our current guidance and updates dependencies to the latest versions.