Increase flexibility of bruteforce signature extration#166
Increase flexibility of bruteforce signature extration#166RonRademaker wants to merge 6 commits into
Conversation
|
Can one of the admins verify this patch? |
|
@mailgun-ci test this please |
obukhov-sergey
left a comment
obukhov-sergey
left a comment
There was a problem hiding this comment.
Lgtm, minor comments, syntax error fails tests
| """ | ||
|
|
||
| @abstractmethod | ||
| def extract_signature(self, message: str): |
There was a problem hiding this comment.
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
| """ | ||
| 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.
I'd skip the parameters since you set the defaults.
| @@ -0,0 +1,195 @@ | |||
| """ | |||
| Module with object oriented approach to | |||
There was a problem hiding this comment.
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? |
obukhov-sergey
left a comment
obukhov-sergey
left a comment
There was a problem hiding this comment.
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. |
3 participants
Allows overriding line length, max line length (useful if you expect disclaimers) and greetings (useful from a language perspective) when extracting signatures bruteforce