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(sed): better support for converting sed patterns #137
Conversation
This PR does: 1. Refactors sed-specific pattern-conversion logic. This logic converts a unix-style pattern (e.g., "s/foo/bar/") into the separate parameters expected by shell.sed() (e.g., new RegExp('foo'), 'bar'). This builds the RegExp more carefully to avoid the issue with escaped slashes (as reported in #136). 2. Explicitly checks for empty patterns and throws an error (previously, these would just silently fail conversion). 3. Adds tests for the previous two cases. 4. Refactors the sed tests to be more maintainable by saving file contents in test variables. No change in the logic of already-existing tests. Fixes #136 Test: 'works with backslashes and forward slashes in pattern' Test: 'does not work with empty regex strings'
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 22 40 +18
=====================================
+ Hits 22 40 +18
Continue to review full report at Codecov.
|
src/shx.js
Outdated
const regexString = match[1].replace(/\\\//g, '/'); | ||
const replacement = match[2].replace(/\\\//g, '/').replace(/\\./g, '.'); | ||
const regexFlags = match[3]; | ||
if (!regexString) { |
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.
Perhaps regexString === ''
would be clearer here.
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.
done
test/specs/cli.js
Outdated
it('does not work with empty regex strings', () => { | ||
(() => { | ||
cli('sed', 's//foo/g', testFileName1); | ||
}).should.throw(Error); |
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.
I would check for the exact error message here, i.e. .should.throw('Bad sed pattern (empty regex)')
.
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.
done
@freitagbr ptal |
LGTM |
This PR does:
a unix-style pattern (e.g., "s/foo/bar/") into the separate
parameters expected by shell.sed() (e.g., new RegExp('foo'), 'bar').
This builds the RegExp more carefully to avoid the issue with
escaped slashes (as reported in Regex support for forward slashes in sed #136).
(previously, these would just silently fail conversion).
contents in test variables. No change in the logic of
already-existing tests.
Fixes #136
Test: 'works with backslashes and forward slashes in pattern'
Test: 'does not work with empty regex strings'