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

Overlapping value replacement #365

Merged
merged 2 commits into from Nov 13, 2017

Conversation

Projects
None yet
2 participants
@tivac
Copy link
Owner

commented Nov 12, 2017

Fixes #363 by sorting values by length so that longer replacements match first.

A little hacky-feeling, but seems ok after thinking about it for a bit.

This commit also solves the recurring travis failures I've been seeing thanks to a tip from @darthmaim in gitter.

WIP: sort values to match by length
So that values with the same prefixes don't stomp over each other. This feels a bit hacky but works for now.

Fixes #363
@pygy

This comment has been minimized.

Copy link

commented on packages/core/plugins/values-replace.js in 2c66d62 Nov 11, 2017

If it can help you feel better about the fix, that was my first idea as well. I think it is optimal as it guarantees that potential prefixes are replaced last, without resorting to string content comparisons that would be costlier.

This comment has been minimized.

Copy link

replied Nov 11, 2017

Looking at the broader context, you may get a speedier matcher by sorting first alphabetically then by length, allowing the regex engine to build an efficient trie (if it can do such a thing). Otherwise you could build a trie-like regex manually.

This comment has been minimized.

Copy link

replied Nov 11, 2017

I was offline this afternoon and whipped up this, only to find out that this lib existed. It has been vetted by Mathias Bynens, I'd rather go with it.

new RegExp('\b' + new RegExpTrie().add(Object.keys(values)).toString() + '\b') will give you an optimized matcher.

One caveat with \b: /\bx\b/ will match -x-, so there may be false positives, depending on what your values can contain.

You may want to go with '([^\w-]|^)(' + new RegExpTrie()... + ')([^\w-]|$), and restore the first/last char when returning.

This comment has been minimized.

Copy link
Owner Author

replied Nov 12, 2017

regex-trie is an interesting approach, I wonder if there'd be much benefit to processing time.

I've never been thrilled about my chosen approach to replacing @value instances, it feels really brittle to me currently.

This comment has been minimized.

Copy link

replied Nov 12, 2017

What's the shape of the node.value fields that are to be replaced? Could one of these hold either a prefix or a suffix of one of the values items (node.value not being a values item)? If so, the use of \b is indeed problematic ('([^\w-]|^)(' + valuesMatcher + ')([^\w-]|$) being a solution).

Explicitly, if values is ['foo'], and node.value is 'hi-foo-bar', you'll get a false positive match.

Otherwise sorting the values by length before matching should be robust AFAICT.

test: Fix up travis failures
- Directly write files instead of copying
- Use travis in VM mode instead of container

@tivac tivac added the pkg:core label Nov 12, 2017

@tivac tivac self-assigned this Nov 12, 2017

@tivac tivac added the bugfix label Nov 12, 2017

@tivac tivac merged commit 1f6fdb5 into master Nov 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tivac tivac deleted the overlapping-value-replacement branch Nov 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.