Simplify dereference string processing#51
Merged
Conversation
The dereference regex (both full string detection and bracket indexing)
originally has two near identical copies to account for any number of
repeating sub-references and a final fully formed attribute. If the
regex has to be modified for new syntax it must be updated in both
locations.
To fix this the regex now expects one or more fully formed attribute
expressions that are delimited by '.' character. To avoid duplication
of the regex again using lookbehind constructs the expression *does*
allow ending a reference string with '.' which is technically malformed.
The logic for processing reference strings consisted of comparing the
set of matches and their spans to see if they are roughly equivalent.
This was both inefficient, overly complicated, and lacked checking for
cycles greater than one step (A->A->A).
An improvement on this is to instead keep a history of each full set of
substitutions made in a list, and after the first pass check if the new
processed string is within the history. This method both greatly
simplifies the check condition if a string is done processing (i.e. it
did not change on the next pass therefore no more substitution can be
made) and if a complex cycle was encountered (e.g. appears in the
history that is not the latest change).
A side benefit to this is our logging info can now include the full
breakdown of substitutions per iteration by repeating that history back
until the final output, which may be useful for debugging. This info is
not controllable at the moment, but may be considered for toggling if it
begins to clutter logs.
A reference string with malformed ending ('.') is allowed and produces
a warning message, with corrections to the string made in situ during
processing.
Test for edge cases of complex cycles and malformed but accepted strings
islas
changed the title
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The dereference regex (both full string detection and bracket indexing) originally has two near identical copies to account for any number of repeating sub-references and a final fully formed attribute. If the regex has to be modified for new syntax it must be updated in both locations.
To fix this the regex now expects one or more fully formed attribute expressions that are delimited by '.' character. To avoid duplication of the regex again using lookbehind constructs the expression does allow ending a reference string with '.' which is technically malformed.
The logic for processing reference strings consisted of comparing the set of matches and their spans to see if they are roughly equivalent. This was both inefficient, overly complicated, and lacked checking for cycles greater than one step (A->A->A).
An improvement on this is to instead keep a history of each full set of substitutions made in a list, and after the first pass check if the new processed string is within the history. This method both greatly simplifies the check condition if a string is done processing (i.e. it did not change on the next pass therefore no more substitution can be made) and if a complex cycle was encountered (e.g. appears in the history that is not the latest change).
A side benefit to this is our logging info can now include the full breakdown of substitutions per iteration by repeating that history back until the final output, which may be useful for debugging. This info is not controllable at the moment, but may be considered for toggling if it begins to clutter logs.
A reference string with malformed ending ('.') is allowed and produces a warning message, with corrections to the string made in situ during processing.
Tests are included to ensure that this simplification addresses previous issues and is an overall improvement.