-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add Repeated option and port re to regex module. #38
base: master
Are you sure you want to change the base?
Conversation
…g. Added examples.
… case to fail with new regex module rules.
I'm a little hesitant to use a non-standard regex parser here, as it constrains textfsm too much. In particular, we are planning to port to other languages which are compatible with the same templates. Use of a regexp engine that is not RE2 compliant (and only RE2 compliant) will jeopardise those plans. |
The regex module is (and plans to be) fully backwards compatible with re. It does allow for some extra functionality, and doesn't python's re already support lookaheads/lookbehinds (?=re) which are not supported by RE2? |
My bad, you meant it to apply to the multiple captures portion that was being leveraged. I'll do some checking to see if this is doable with pyre2. |
According to https://github.com/google/re2/wiki/Syntax re2 would not be an option |
… both re and regex in python 3.6
… with and without regex module.
…Ensured error is raised when using Repeated keyword without regex module.
…havior. Refactored test for proper error functionality when re and Repeated are used.
…for correct behavior.
…ut regex module in python 3.6 and python 2.7
|
… Works in 2.7 and 3.6
@@ -48,6 +48,6 @@ | |||
packages=['textfsm'], | |||
include_package_data=True, | |||
package_data={'textfsm': ['../testdata/*']}, | |||
install_requires=['six', 'future'], | |||
install_requires=['six', 'future', 'regex'], |
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'm conflicted here ... we want to opportunistically install regex but not necessarily require it, as only newer templates would need it. Unfortunately I see no provision in setup.py for a install_desires=... stanza, so I guess this will have to do?
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.
Its the templates that drive the dependency. i.e. only if there are templates with the Repeated keyword, do we need the regexp module installed (plus the relevant unittests). It could be a declared as required as part of a template package install rather than here .. but that is perhaps asking to much of the template maintainers.
Fixes #18
Instances of re were changed to regex. This allows for increased matching power.
Repeated data parsing as asked for in issue #18 was added using the new regex module functionality. Test cases were updated to reflect the increased functionality and new option.