[proofing] Fully clean up search/replace UX #499
Conversation
|
@kvchitrapu I made some substantial changes here so that the UX cleanup could go a little smoothly. I think this preserves your original vision but I'm curious what you think of the new changes. Edit -- I should also mention that I love the logging but that I found it very difficult to read the code with them. Would love to find a balance between logging and keeping the code readable. |
There was a problem hiding this comment.
I prefer personally reviewing changes to ensure accuracy. By conducting double-checks, we give proofers ample opportunities to thoroughly review their work, especially when dealing with large batches of changes.
In my opinion, relying solely on results without validation could potentially lead to errors. For instance, I do not simply run a "search-replace" all in an editor when renaming a variable. Instead, I go through each line to ensure precision.
However, I do recognize the convenience that this new approach offers. I am open to either method, as long as we are aware of the trade-offs involved.
| }, | ||
| ) | ||
| assert resp.status_code == 200 | ||
| assert "Found 12 matching lines for query" in resp.text |
There was a problem hiding this comment.
This is good work! We now have test data to validate these changes.
| @@ -391,216 +413,98 @@ def replace(slug): | |||
| if project_ is None: | |||
| abort(404) | |||
|
|
|||
| form = ReplaceForm(request.form) | |||
| form = ReplaceForm(request.args) | |||
There was a problem hiding this comment.
Is request.args your preferred way to pass params to the server? After experimenting a bit, I came out in favor of form. At some point args will break as the query or replace string grows in size or perhaps has sensitive info or has data unsupported in the url. Perhaps I'm missing your reasoning here.
There was a problem hiding this comment.
I think for most cases, query and replace will be small. For small form submits where the information is not sensitive and where there is no database state change, GET with request.args is the common pattern.
There was a problem hiding this comment.
You see
Thanks for the review!
For instance, I do not simply run a "search-replace" all in an editor when renaming a variable. Instead, I go through each line to ensure precision.
I agree that a blind search/replace is unwise and that the user must review each change to ensure quality. My feeling is that the phase where the user checks/unchecks each item in the result set accomplishes this purpose already. I'm curious why you feel differently.
In my opinion, it's easy to verify the checks/unchecks when there are only a handful of matches on a single search+replace page. However, as the number of matches increases (for example, hundreds of matches), the pagination will spread the checks/unchecks across multiple search+replace pages. This can make it difficult to keep track of which checks/unchecks have already been applied, leading to potential tracking issues.
| def marked_replace(self) -> str: | ||
| buf = [] | ||
| for i, t in enumerate(self.splits): | ||
| if i % 2 == 1: |
There was a problem hiding this comment.
This odd/even heuristic may not always work. I believe what you are saying is query is a delimiter and the resulting splits will always have regex matched elements in odd indices. The regex will escape the odd/even structure. If you want to go ahead, put an assert that odd index matches the query/regex pattern.
Also, this is one reason it is best to review/confirm the changes before committing. The confirm page will be a nano-version of what PR is for software.
There was a problem hiding this comment.
I'll add an assert. This is an invariant guaranteed by the Python standard library:
If there are capturing groups in the separator and it matches at the start of the string, the result will start with an empty string. [...] That way, separator components are always found at the same relative indices within the result list.
(emphasis added)
|
Thanks for the review!
I agree that a blind search/replace is unwise and that the user must review each change to ensure quality. My feeling is that the phase where the user checks/unchecks each item in the result set accomplishes this purpose already. I'm curious why you feel differently. |
|
Sorry @akprasad . I intended to get back to my regular routine last week, but I couldn't find time to unblock my schedule. I'll get back on track this week. |
|
@akprasad , go forward with this PR and land it. Consider adding an assert to catch lines that escape odd/even structure. Record user's |
These changes build on @kvchitrapu's prior work and simplify it slightly.
Specifically, this makes the following changes:
_find_replacementsutility function that does the regex searching, etc. By using dataclasses, we can reuse this one function for the full flow.Proposed flow: