-
Notifications
You must be signed in to change notification settings - Fork 505
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
fix: replace regex used to split ranges #434
Conversation
Found in #433, the regex used to split ranges on `||` was padded by `\s*` on either side causing a decrease in performance when used on range strings with a lot of spaces. Since the result of the split is immediately trimmed, we can just split on the string instead.
I ran some benchmarks locally using the following script which passes different arguments to const Range = require('./classes/range')
const a = process.argv.slice(2)
const s = ' '.repeat(Number(a[0]))
const r = `${s}1.1.1${s}`
const v = `${r}||${r}`.repeat(Number(a[1]))
const split = ({
poly: /\s*\|\|\s*/,
regex: /\|\|/,
string: '||',
})[a[2]]
new Range(v, {}, split) 5 spaces x 10 versions
100 spaces x 100 versions
1000 spaces x 1000 versions
|
This adds `@npmcli/template-oss` to manage GitHub Actions, linting, and other chores. It specifically pins to the latest version of the library in order to allow for the following manual changes: - Files outside of `lib/` to avoid breaking public API - Keeping engines (and testing) on `>=10` - Installs `npm@7` in CI to work with node 10 This surfaced a few bugs which I opted to fix in separate issues: - #432 - #434
This adds `@npmcli/template-oss` to manage GitHub Actions, linting, and other chores. It specifically pins to the latest version of the library in order to allow for the following manual changes: - Files outside of `lib/` to avoid breaking public API - Keeping engines (and testing) on `>=10` - Installs `npm@7` in CI to work with node 10 This surfaced a few bugs which I opted to fix in separate issues: - #432 - #434
This adds `@npmcli/template-oss` to manage GitHub Actions, linting, and other chores. It specifically pins to the latest version of the library in order to allow for the following manual changes: - Files outside of `lib/` to avoid breaking public API - Keeping engines (and testing) on `>=10` - Installs `npm@7` in CI to work with node 10 This surfaced a few bugs which I opted to fix in separate issues: - #432 - #434
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is very clearly correct 👍
Found in #433, the regex used to split ranges on `||` was padded by `\s*` on either side causing a decrease in performance when used on range strings with a lot of spaces. Since the result of the split is immediately trimmed, we can just split on the string instead.
Found in #433, the regex used to split ranges on `||` was padded by `\s*` on either side causing a decrease in performance when used on range strings with a lot of spaces. Since the result of the split is immediately trimmed, we can just split on the string instead.
Found in #433, the regex used to split ranges on
||
was padded by\s*
on either side causing a decrease in performance when used on range
strings with a lot of spaces. Since the result of the split is
immediately trimmed, we can just split on the string instead.