-
Notifications
You must be signed in to change notification settings - Fork 285
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
Increase flexibility of bruteforce signature extration #166
base: master
Are you sure you want to change the base?
Increase flexibility of bruteforce signature extration #166
Conversation
Can one of the admins verify this patch? |
@mailgun-ci test this please |
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, minor comments, syntax error fails tests
talon/signature/extractor.py
Outdated
""" | ||
|
||
@abstractmethod | ||
def extract_signature(self, message: str): |
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.
nosetests --with-xunit --with-coverage --cover-xml --cover-package=talon
EE
======================================================================
ERROR: Failure: SyntaxError (invalid syntax (extractor.py, line 36))
----------------------------------------------------------------------
Traceback (most recent call last):
File "/var/lib/jenkins/shiningpanda/jobs/b20cb595/virtualenvs/d41d8cd9/local/lib/python2.7/site-packages/nose/loader.py", line 418, in loadTestsFromName
addr.filename, addr.module)
...
from talon.signature.extractor import BruteForceExtractor
File "/var/lib/jenkins/workspace/lib-py-talon-pr/talon/signature/extractor.py", line 36
def extract_signature(self, message: str):
^
SyntaxError: invalid syntax
talon/signature/bruteforce.py
Outdated
@@ -183,5 +61,5 @@ def _process_marked_candidate_indexes(candidate, markers): | |||
>>> _process_marked_candidate_indexes([9, 12, 14, 15, 17], 'clddc') | |||
[15, 17] | |||
""" | |||
match = RE_SIGNATURE_CANDIDATE.match(markers[::-1]) | |||
return candidate[-match.end('candidate'):] if match else [] | |||
brute_force_extractor = BruteForceExtractor(max_lines=SIGNATURE_MAX_LINES, max_line_length=TOO_LONG_SIGNATURE_LINE) |
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.
I'd skip the parameters since you set the defaults.
talon/signature/extractor.py
Outdated
@@ -0,0 +1,195 @@ | |||
""" | |||
Module with object oriented approach to |
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.
Out of curiosity - why the comment lines are so short? I know shorter lines are easier to read but usually 80 chars is ok. Functions that go on for many lines become hard to read just like lines stretching for many chars.
Thanks for the feedback, I processed it |
@mailgun-ci test this please |
@RonRademaker sorry for delay with the merge, can you plz add abc to setup.py to make tests pass? |
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.
Tests fail with ImportError: cannot import name ABC
. Plz add abc to setup.py.
Any way I can locally run the tests to see if they pass before pushing changes? |
@mailgun-ci test this please |
@RonRademaker sure, just run nosetests from the repo folder, you might need to "pip install nose" and "pip install mock", they're also in test_require part of setup.py; we plan to migrate to travis ci as well, so that you don't have to run tests yourself locally. |
Allows overriding line length, max line length (useful if you expect disclaimers) and greetings (useful from a language perspective) when extracting signatures bruteforce