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

Refactor MI parsing to decrease the number of regex matches to perform #72

Merged
merged 1 commit into from
Aug 13, 2022

Conversation

barisione
Copy link
Collaborator

  • I have added an entry to CHANGELOG.md

Summary of changes

The previous code to match MI records used to do matches twice. This PR simplifies the code (hopefully!) and means that matches need to happen only once.
Functionally, there should be no changes as I kept the regexes mostly identical and the same checks are executed in the same order.
“Mostly” because tokens used to be matched with (\d*) and are now matched with (\d+)? which I think makes it clearer to readers of the code that tokens are optional.

Test plan

Tested by running

nox -s tests

@barisione barisione marked this pull request as ready for review August 7, 2022 17:38
Copy link
Owner

@cs01 cs01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIce, this is much better!

Should we consider writing a grammar to simplify this more? Someone did this in rust for pygdbmi -- https://github.com/JP3BGY/gdbmi/blob/master/src/gdbmi_output.pest

We could use lark which has implementation in other languages for the same source .lark grammar.

@barisione
Copy link
Collaborator Author

Should we consider writing a grammar to simplify this more? Someone did this in rust for pygdbmi -- https://github.com/JP3BGY/gdbmi/blob/master/src/gdbmi_output.pest

Good idea. I opened #75.

@barisione barisione merged commit 3130f55 into cs01:master Aug 13, 2022
@barisione barisione deleted the refactor-matching branch August 13, 2022 08:57
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.

None yet

2 participants