Skip to content

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
Contributor

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.

@harro
Copy link
Collaborator

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.

include_package_data=True,
package_data={'textfsm': ['../testdata/*']},
install_requires=['six', 'future'],
install_requires=['six', 'future', 'regex'],
Copy link
Collaborator

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
Collaborator

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.

@harro harro requested a review from buxtronix July 25, 2019 04:30
jhogg added a commit to jhogg/textfsmplus that referenced this pull request Dec 17, 2025
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