smartparens icon indicating copy to clipboard operation
smartparens copied to clipboard

sp-get-string does not recognize string fences

Open ThibautVerron opened this issue 4 years ago • 5 comments

Hi,

Expected behavior

Be able to treat "generalized strings" (delimited by characters with the syntax property string-fence) as paired expression.

Actual behavior

Such strings are not recognized without ad-hoc configuration.

Steps to reproduce the problem

In a ruby-mode buffer, with |%w(test) (string syntax), sp-forward-sexp results in %w|(test) instead of %w(test)|.

I'm not a ruby user (I noticed the same problem with another major-mode I am maintaining) and I realize by browsing the code that smartparens supplies another forward-sexp function for ruby, which does take care of that. But it is not sufficient for all the sp functionality, for example the delimiters of the string are not highlighted.

It seems that sp-get-string only detects standard strings, but that sp-get-quoted-string-bounds does detect those strings with arbitrary delimiters. So it should be possible to handle such strings everywhere, no?

Environment & version information

  • smartparens version: 1.11.0 (commit 555626a4)
  • Active major-mode: ruby-mode
  • Smartparens strict mode: nil
  • Emacs version (M-x emacs-version): GNU Emacs 26.3 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.11) of 2019-09-16
  • Starterkit/Distribution: Vanilla
  • OS: gnu/linux

ThibautVerron avatar Jun 11 '20 15:06 ThibautVerron

After looking at it more closely, it looks like sp-get-string does recognize such generalized strings, then passes it on to sp--get-string, which then... chooses to discard it? Why?

https://github.com/Fuco1/smartparens/blob/555626a43f9bb1985aa9a0eb675f2b88b29702c8/smartparens.el#L5355-L5357

ThibautVerron avatar Jun 12 '20 07:06 ThibautVerron

Okay. It seems that I can get the functionality I want by removing this test (and replacing :op cl with :op op), and changing sp-get-thing as follows:

  • replacing

    (eq (syntax-class (syntax-after (point))) 7)

    with

    (memq (syntax-class (syntax-after (point))) '(7 15))

  • moving the whole block up in the condition list (because otherwise the string delimiters can be recognized as sexp delimiters)

I will try to understand how the testing suite works and make a pull request if things go alright (spoiler: they won't).

ThibautVerron avatar Jun 12 '20 07:06 ThibautVerron

Ok so basic functionality (e.g. sp-forward-sexp) seems to work and no test break: https://github.com/ThibautVerron/smartparens/tree/feature/generic-string

I added tests in ruby based on the existing ones, I had to tweak sp-get-string to ignore comments when point is on the boundary of the comments. Again, it doesn't seem to have broken anything.

Other features such as sp-down-sexp don't seem to work, I will have a look when I get the chance.

Should I make a pull request now, or wait until everything works? As it is, it might be slightly breaking for ruby users, for example sp-backward-sexp with %w(asd)| will now move the point to before %, not before the opening paren.

ThibautVerron avatar Jun 12 '20 08:06 ThibautVerron

Sorry for taking so much time to get back to this. Your approach seems good, if you have some code it would be best to open a pull request which I can review or expand. I am eternally grateful for any help as I've been quite depressed these past couple months and so the work on my open source projects came to a halt basically.

If it works for you and you add a testcase (or multiple) I'm now following the optimistic merging approach and so we can simply include your code and wait until someone complains (and then either fix it or revert it).

Fuco1 avatar Oct 07 '20 17:10 Fuco1

No problem at all !

I have been using my code without any problem, so I think I can open a pull request now. Otoh I have not done any progress towards fixing the problems I mentioned before (I don't use down-sexp or ruby, so I selfishly forgot about those issues).

Cheers, and take care! :)

ThibautVerron avatar Oct 08 '20 09:10 ThibautVerron