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

Add Repeated option and port re to regex module. #38

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

Conversation

gachteme
Copy link

@gachteme gachteme commented Oct 15, 2018

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.

@buxtronix
Copy link
Collaborator

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.

@gachteme
Copy link
Author

gachteme commented Nov 13, 2018

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?
I can see a reason to be hesitant with using any third-party packages though.
If absolute consistency (IE: throwing errors when something is supported in the language used but not all supported languages) between languages is desired a check could be added for regex RE2 compliance.

@gachteme
Copy link
Author

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.

clitable.py Outdated Show resolved Hide resolved
clitable.py Outdated Show resolved Hide resolved
textfsm.py Outdated Show resolved Hide resolved
textfsm.py Outdated Show resolved Hide resolved
textfsm_test.py Outdated Show resolved Hide resolved
textfsm_test.py Outdated Show resolved Hide resolved
textfsm_test.py Outdated Show resolved Hide resolved
@harro
Copy link
Contributor

harro commented Jul 10, 2019

According to https://github.com/google/re2/wiki/Syntax re2 would not be an option

@gachteme
Copy link
Author

gachteme commented Jul 25, 2019

I'm seeing the merge from #55 causes errors in python 2.7, is that no longer supported?
If this is the case then
this should be ready for review. The only caveat is that I don't have an automated way of running the test suite with an environment containing the re module versus the regex module. I have manually run the tests in both environments.

@@ -48,6 +48,6 @@
packages=['textfsm'],
include_package_data=True,
package_data={'textfsm': ['../testdata/*']},
install_requires=['six', 'future'],
install_requires=['six', 'future', 'regex'],
Copy link
Contributor

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?

Copy link
Contributor

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.

tests/clitable_test.py Outdated Show resolved Hide resolved
@harro harro requested a review from buxtronix July 25, 2019 04:30
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.

Proposals
3 participants