Add logging of search-replace transformations.#35
Add logging of search-replace transformations.#35
Conversation
|
@gitlost Can you share a screencast? |
|
Did an asciinema https://asciinema.org/a/140798 |
|
@gitlost That's pretty fantastic. |
|
Shucks, I'll request a review so! |
|
Let's get a couple more reviews on this too. |
danielbachhuber
left a comment
There was a problem hiding this comment.
Some small nits about usage; otherwise, this looks great :)
| * '%8' Reverse | ||
| * '%U' Underline | ||
| * '%F' Blink (unlikely to work) | ||
| * They can be concatenated. |
There was a problem hiding this comment.
"They can be concatenated" needs an additional newline.
There was a problem hiding this comment.
This whole color stuff can now be just dropped.
| * Warning: causes a significant slow down, similar or worse to enabling --precise or --regex. | ||
| * | ||
| * [--log_context=<before_num,after_num>] | ||
| * : Number of characters to display before and after the match. One number sets both before and after. Two comma-separated numbers set before and after respectively. Defaults to 40. Ignored if not logging. |
There was a problem hiding this comment.
- For consistency, should we follow
--before_context=<num>and--after_context=<num>as established bywp db search? I rather like--log_context=<>better, but I think we should opt for consistency whenever possible. - We should wrap the argument description at whatever count we use for the others.
There was a problem hiding this comment.
we should opt for consistency
Am going to propose adding the same "shortcut" version --context to wp db search (also a shortcut ---colors). This would make them more or less consistent (barring the log_ prefix), with db search having a superset.
(The main motivation for not just using log_context_before etc was as you mention below the large amount of options that search-replace already has.)
The log_ prefix seemed needed to give clarity to and group the options and is sort of consistent with db search (which doesn't have a log option) but I'll drop if you think it's confusing/inconsistent.
Edit okay given the other log options are being dropped, I'll drop --log_context and just do --context_before and --context_after.
wrap the argument description
Will do.
(Probably the same/similar newline stripping as is now done for the handbook should be done for help in general, as the some-do, some-don't wrapping currently on help looks odd.)
| * [--log_context=<before_num,after_num>] | ||
| * : Number of characters to display before and after the match. One number sets both before and after. Two comma-separated numbers set before and after respectively. Defaults to 40. Ignored if not logging. | ||
| * | ||
| * [--log_prefixes=<old_prefix,new_prefix>] |
There was a problem hiding this comment.
Is this necessary as a command argument, or could it be an environment variable instead?
The command arguments available to wp search-replace have become a bit overwhelming. We may need to more actively address this in the future.
There was a problem hiding this comment.
It's easily changed to be an environment variable instead, so will do.
The command arguments available to wp search-replace have become a bit overwhelming.
Yes, the synopsis isn't easy to parse, especially with all those square brackets. Was thinking some auto-embolding (similar to most Unix man pages) would help but not sure:
wp search-replace <old> <new> [<table>...] [--dry-run] [--network] [--all-tables-with-prefix] [--all-tables] [--export[=]] [--export_insert_size=] [--skip-columns=]
[--include-columns=] [--precise] [--recurse-objects] [--verbose] [--regex] [--regex-flags=] [--regex-delimiter=] [--format=] [--report]
[--report-changed-only] [--log[=]] [--log_context=<before_num,after_num>] [--log_prefixes=<old_prefix,new_prefix>] [--log_colors=<table_column_id_color,old_color,new_color>]
and similarly for the line-by-line options. I actually prototyped something like this a long time ago (when doing the word-wrapping) so will dig it up if you think it's worthwhile.
| * [--log_prefixes=<old_prefix,new_prefix>] | ||
| * : The old and new prefixes to prepend to the log lines for old match and new replacement, comma-separated. Defaults to '< ,> '. Use ',' for no prefixes. Ignored if not logging. | ||
| * | ||
| * [--log_colors=<table_column_id_color,old_color,new_color>] |
There was a problem hiding this comment.
Same comment as above: does this need to be a command argument?
I think my preference would be to remove this as an option entirely. We can bring it up again in the future if anyone brings it up.
|
💯 Great work on this, @gitlost. |
|
Finally got around to test this (didn't have enough data bandwidth for the Composer update yesterday). I agree, @gitlost, this is great, and a huge improvement on the existing behaviour for more risky replacements. |
Add logging of search-replace transformations.
Fixes #1
Adds logging of before/after changes.