-
Notifications
You must be signed in to change notification settings - Fork 65
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
problems with GNFA.to_regex #122
Comments
@colette-b Thank you for the report! I can reproduce the issue on my end, as well.
@abhinavsinha-adrino I believe the GNFA class was your addition to the library. Do you have any thoughts on this? cc @eliotwrobson (in case you have anything to add) |
@caleb531 I think it was @abhinavsinha-adrino that added GNFA (in PR #46). I'm not super familiar with the way that the GNFA works; though I did do some light refactoring of the I played with the example code and it looks like the dfa produced correctly recognizes the |
@eliotwrobson Oh yes, my apologies. Unfortunately, it does appear that this bug was introduced in the v7 release, because the bug is not present in v6:
Here is a link to the diff between v6 and v7, for reference: |
Ah then that is my bad then 😅 I'll add this to the test suite and see if I can whip up a fix really quickly. EDIT: it seems like this issue might not come up if you try converting directly from the NFA to the GNFA (no intermediate DFA)? Will play with it more to be sure but kinda weird. |
@eliotwrobson I think I found one of the problems (hopefully the only one 🥲, I haven't run anything yet) This must have been generating errors with many other regular expressions. |
@abhinavsinha-adrino ahhh yeah that would do it, thank you! Let me change that and see if it works. One of the other issues is in the code below: Lines 251 to 259 in f7acee8
This test will never raise an error, which is probably why we didn't catch this bug earlier. |
@eliotwrobson I did a lousy job in testing the code. This was my first open-source contribution, and I'm not a programmer, so I kind of hated the testing part 😖. Maybe some more tests need to be added. I guess we can simply add 'a*' test case. Is it possible for you to add this with the fix? |
@abhinavsinha-adrino take a look at the PR I just put up (#123), it adds this along with the ability to easily add arbitrary regex to the test suite. |
@eliotwrobson 👍, this fixes the issue. |
Excellent! I have merged that PR, and am presently pushing a Automata v7.1.0 release containing this fix (as well as the other recent additions, like the |
@colette-b Alright, v7.1.0 is released with this fix. Thank you for reporting this bug! https://github.com/caleb531/automata/releases/tag/v7.1.0 I will be closing this issue now, but please let me know if you run into any issues. |
GNFA.to_regex seems to give wrong results sometimes. For example, the following code:
prints a single-character string
*
, which is not a valid regex.The text was updated successfully, but these errors were encountered: