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

Increase flexibility of bruteforce signature extration #166

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

RonRademaker
Copy link

Allows overriding line length, max line length (useful if you expect disclaimers) and greetings (useful from a language perspective) when extracting signatures bruteforce

@mailgun-ci
Copy link

Can one of the admins verify this patch?

@obukhov-sergey
Copy link
Member

@mailgun-ci test this please

Copy link
Member

@obukhov-sergey obukhov-sergey left a 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

"""

@abstractmethod
def extract_signature(self, message: str):
Copy link
Member

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

@@ -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)
Copy link
Member

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.

@@ -0,0 +1,195 @@
"""
Module with object oriented approach to
Copy link
Member

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.

@RonRademaker
Copy link
Author

Thanks for the feedback, I processed it

@obukhov-sergey
Copy link
Member

@mailgun-ci test this please

@obukhov-sergey
Copy link
Member

@RonRademaker sorry for delay with the merge, can you plz add abc to setup.py to make tests pass?

Copy link
Member

@obukhov-sergey obukhov-sergey left a 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.

@RonRademaker
Copy link
Author

Any way I can locally run the tests to see if they pass before pushing changes?

@obukhov-sergey
Copy link
Member

@mailgun-ci test this please

@obukhov-sergey
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants