-
Notifications
You must be signed in to change notification settings - Fork 92
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
Optionally use sed extended regex syntax #453
Optionally use sed extended regex syntax #453
Conversation
// abc[123]def | ||
|
||
main { | ||
echo replace_regex("abc123def", "([0-9]+)", "[\1]", true) |
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 test would fail on a machine that does not support the new flag, right? Can we at least write a note somewhere or maybe even skip this test?
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 prefer to keep the test, because it confirms that on systems which do support -E
or -r
, the right option is being used. And while Amber may be run on older systems, I doubt that anyone will run cargo test
on them. Would you be happy with a comment in the test file?
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.
Yeah... I think that we should create some syntax for interfacing commands given different versions and os's. This way we could just run the sed through that interface and never worry
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 think that we should create some syntax for interfacing commands given different versions and os's
Makes sense, would need to give it some thought, but happy to discuss. In the meantime, I've added this warning comment. Does this resolve your immediate request?
// This will fail on any system where sed does not support extended
// regex syntax, via "-r" on GNU sed and "-E" on all other versions.
29d8456
to
58e20b9
Compare
58e20b9
to
4b40372
Compare
Co-authored-by: Phoenix Himself <[email protected]>
4b40372
to
aed7346
Compare
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.
LGTM
* Optionally use sed extended regex syntax. * Update src/std/text.ab Co-authored-by: Phoenix Himself <[email protected]> * Add comment stating that sed extended regex may fail on some older Linux variants. * Change regex used to detect GNU sed. * Remove duplicate unsafe from stdlib function. --------- Co-authored-by: Phoenix Himself <[email protected]>
Fixes #444.