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

kaizen: clean up state finite automata #308

Merged
merged 2 commits into from
May 31, 2024
Merged

kaizen: clean up state finite automata #308

merged 2 commits into from
May 31, 2024

Conversation

timbray
Copy link
Owner

@timbray timbray commented May 25, 2024

addresses #197

This gets rid of the DFA/NFA distinction, everything is now an NFA. The stretch goal is to fix #188, but the messy sprawl of DFA-and-NFA was getting in the way of understanding.

This includes a bunch of commented-out code for pretty-printing state machines, which I found totally essential in migrating to all-NFA. I'm going to create an issue to bring this code out of comments and make it possible to switch it off/on to facilitate the creating and merging different kinds of patterns.

addresses #197

Signed-off-by: Tim Bray <tbray@textuality.com>
@timbray
Copy link
Owner Author

timbray commented May 25, 2024

I see that I accidentally lost a couple of unit tests, although the coverage didn't go down. Will clean this up subsequently. There's a huge amount of quite difficult work here that I want to get checked in so I have a stable base to continue work on #188 and then some new features.

Signed-off-by: Tim Bray <tbray@textuality.com>
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 97.80220% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 96.32%. Comparing base (a716bc1) to head (8351137).
Report is 68 commits behind head on main.

Files Patch % Lines
nfa.go 94.82% 1 Missing and 2 partials ⚠️
shell_style.go 95.45% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
+ Coverage   96.19%   96.32%   +0.13%     
==========================================
  Files          17       17              
  Lines        2129     1634     -495     
==========================================
- Hits         2048     1574     -474     
+ Misses         54       34      -20     
+ Partials       27       26       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timbray
Copy link
Owner Author

timbray commented May 30, 2024

Having thought about it, I can see why this is a difficult PR to review. It's all down in the finite-automaton weeds and not that many people care about this stuff or have exposure to it. Having said that, a bunch of ugly code was removed and the tests all still pass and I see no performance regression. So I will probably break the rules and go ahead and merge if nobody is up to doing a real review in the next day or so.

@embano1
Copy link
Collaborator

embano1 commented May 31, 2024

+1

@timbray timbray merged commit 49e31ba into main May 31, 2024
7 checks passed
@timbray timbray deleted the all-nfa branch May 31, 2024 15:28
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.

fix: state-machine size explosion on wildcards
3 participants