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

fill-paragraph breaks code structure #9

Closed
andreyorst opened this issue Jun 11, 2020 · 11 comments
Closed

fill-paragraph breaks code structure #9

andreyorst opened this issue Jun 11, 2020 · 11 comments
Labels
bug Something isn't working

Comments

@andreyorst
Copy link
Contributor

Sorry if I'm overwhelming you with issues, but I've finally got my hands on your package, and had put it to the test.

Here's the recording https://asciinema.org/a/ykl6Brm5oH8K49s5R45Elsw25

What I'm doing is this: when cursor is outside the doc string I'm pressing C-q that runs fill-paragraph function, which hard wraps text. This works correct for both cases when cursor is before doc string and after doc string. But when it is inside doc string the structure of above code changes.

This doesn't seem to happen in non Clojure buffers. Here's example code:

(ns some-namespace.core
  (:import [java.util.concurrent Executors])
  (:require [clojure.string :as string]))

(defn foo
  "pretty long docsting that will be affected by `fill-paragraph` function"
  [x]
  x)
@justinbarclay
Copy link
Owner

No need to apologize, these are all good issues. These videos are very insightful too, smart-mode is a complex beast so it helps to see where it's breaking.

@justinbarclay
Copy link
Owner

Note for myself when I can look into this: Clojure mode overwrites how fill paragraph works, specifically when it is called inside of a string. It narrows to region on the string block, runs some transformation on text, and then widens to the buffer.

This is troublesome for parinfer-rust-mode because while detecting changes, if a buffer is narrowed, it will see those changes as existing within that narrowed scope and not within the entire file/buffer. So in the example above it will see the changes to the doc string on line 1 instead of line 5. This can be exasperated by it also using the old buffer text, from the full file, and comparing it against the text in the narrowed region/buffer. This will send very confusing data to parinfer-rust.

@justinbarclay justinbarclay added the bug Something isn't working label Jun 16, 2020
@justinbarclay
Copy link
Owner

Hey, I've come back to this issue and while running clojure-mode and parinfer-rust-mode (43a49d7), I'm no longer able to reproduce. Are you still running into issues with fill-paragraph?

@andreyorst
Copy link
Contributor Author

Still happens to me with the example code I've provided

@justinbarclay
Copy link
Owner

Thank you, that answer helped me figure out why I couldn't reproduce it. I have since been able to reproduce it and come up with a potential fix. This is on another branch that I am going to test over the next couple of days. I'll update this issue when it gets merged in.

@justinbarclay
Copy link
Owner

This issue should be resolved for real this time

@andreyorst
Copy link
Contributor Author

seems to work correct even for narrowed regions. I think it is solved! If I encounter any other place where this happens I'll reopen, but I've tested all places where I've seen this issue, and all of those seem to work fine.

@andreyorst
Copy link
Contributor Author

It happened again. I'll post repro later

@andreyorst andreyorst reopened this Jul 14, 2020
@andreyorst
Copy link
Contributor Author

@justinbarclay actually, the initial recipe reproduces the issue

@justinbarclay
Copy link
Owner

justinbarclay commented Jul 21, 2020

I was able to recreate this issue and fixed it with 967dfa9 (which is why I think my tests for this case passed but didn't work in a non test environment)

@andreyorst
Copy link
Contributor Author

I was able to recreate this issue and fixed it with 967dfa9 (which is why I think my tests for this case passed but didn't work in a non test environment)

yes, it works now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants