Skip to content
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

paredit/kill-one-at-pos when deleting word in string/comment is questionable #343

Closed
lread opened this issue Feb 10, 2025 · 0 comments · Fixed by #358
Closed

paredit/kill-one-at-pos when deleting word in string/comment is questionable #343

lread opened this issue Feb 10, 2025 · 0 comments · Fixed by #358

Comments

@lread
Copy link
Collaborator

lread commented Feb 10, 2025

Version
1.1.49

Platform
All

Symptom
kill-one-at-pos wants to kill a word w within a string/comment when w's col position spans requested col.

Reproduction

(require '[rewrite-clj.paredit :as pe]
         '[rewrite-clj.zip :as z])

;                 11111111112222
;        12345678901234567890123
(let [s "foo  ;   hello   world"
      zloc (z/of-string* s {:track-position? true})
      row 1]
  (for [col (range 1 (inc (count s)))]
    [(format "%02d" col)
     (-> zloc
         (pe/kill-one-at-pos {:row row :col col})
         (z/root-string))]))

Actual behavior

(let [s "foo  ;   hello   world"
      zloc (z/of-string* s {:track-position? true})
      row 1]
  (for [col (range 1 (inc (count s)))]
    [(format "%02d" col)
     (-> zloc
         (pe/kill-one-at-pos {:row row :col col})
         (z/root-string))]))
;; => (["01" ";   hello   world"]          good, we are in span of foo, so delete it
;;     ["02" ";   hello   world"]          "
;;     ["03" ";   hello   world"]          "
;;     ["04" "foo"]                        good, we search forward for the next non-ws none after col and delete it
;;     ["05" "foo"]                        "
;;     ["06" "foo"]                        good, we are at comment and delete it
;;     ["07" "foo  ;   hello   world"]     ok, guess if we are not within a word we do not delete it
;;     ["08" "foo  ;   hello   world"]     "
;;     ["09" "foo  ;      world"]          BAD, hello starts at col 10, but we've deleted it
;;     ["10" "foo  ;      world"]          good, we are within hello
;;     ["11" "foo  ;      world"]          "
;;     ["12" "foo  ;      world"]          "
;;     ["13" "foo  ;      world"]          "
;;     ["14" "foo  ;      world"]          "
;;     ["15" "foo  ;   hello   world"]     again, if we are between words, no-op
;;     ["16" "foo  ;   hello   world"]     "
;;     ["17" "foo  ;   hello   "]          BAD, world starts at 18
;;     ["18" "foo  ;   hello   "]
;;     ["19" "foo  ;   hello   "]
;;     ["20" "foo  ;   hello   "]
;;     ["21" "foo  ;   hello   "]
;;     ["22" "foo  ;   hello   "])

Expected behavior
If I study the code, I think the intent almost matches the above and we have an off-by-one-error.

See also
Killing a word within a string uses same algorithm and has same problem:

;                  111111111122 2
;        1234 56789012345678901 2
(let [s "foo \"   hello   world\""
      zloc (z/of-string* s {:track-position? true})
      row 1]
  (for [col (range 1 (inc (count s)))]
    [(format "%02d" col)
     (-> zloc
         (pe/kill-one-at-pos {:row row :col col})
         (z/root-string))]))
;; => (["01" "\"   hello   world\""]
;;     ["02" "\"   hello   world\""]
;;     ["03" "\"   hello   world\""]
;;     ["04" "foo"]
;;     ["05" "foo"]
;;     ["06" "foo \"   hello   world\""]
;;     ["07" "foo \"   hello   world\""]
;;     ["08" "foo \"      world\""]            BAD, hello starts at col 9 but we've deleted it
;;     ["09" "foo \"      world\""]
;;     ["10" "foo \"      world\""]
;;     ["11" "foo \"      world\""]
;;     ["12" "foo \"      world\""]
;;     ["13" "foo \"      world\""]
;;     ["14" "foo \"   hello   world\""]
;;     ["15" "foo \"   hello   world\""]
;;     ["16" "foo \"   hello   \""]           BAD, world starts at col 17 but we've deleted it
;;     ["17" "foo \"   hello   \""]
;;     ["18" "foo \"   hello   \""]
;;     ["19" "foo \"   hello   \""]
;;     ["20" "foo \"   hello   \""]
;;     ["21" "foo \"   hello   \""]
;;     ["22" "foo \"\""])

Diagnosis
My feeling is that this fn does too much.
Also I'm not sure of the general usefulness of kill-one-at-pos within a string/comment is:

  • Its concept of a "word" is simplistic.
  • It doesn't handle removing whitespace after the killed word.

Action
I'll fix the off-by-one error.
If folks try to use this fn and have difficulty we can raise a separate issue to try to addresss.

lread added a commit that referenced this issue Feb 18, 2025
In-string handling now:
- no-op if pos does not locate to word (i.e. whitespace)
- no longer has off-by-one error for word deletion

Closes #343
@lread lread closed this as completed in 78bb005 Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant