parinfer-rust icon indicating copy to clipboard operation
parinfer-rust copied to clipboard

Emacs respect string delimiter default

Open gagbo opened this issue 2 years ago • 8 comments

Hello,

Following the discussion in #139 I wanted to provide a potential fix so everyone sees what I meant. There are mainly 2 "big" things and a small thing changing in this PR:

  • When calling parinfer-make-option in Emacs, instead of producing an empty option, it produces the default option (coming from cli_options and the serde default values in types::Options)
  • The Emacs wrapper now exposes getters and setters to change the options on the fly, since it seems to be the only way to set/prepare options before sending a request (this will be useful to allow parinfer-rust-mode downstream to adapt to Janet/Guile/Yuck etc.)
  • I added a rustfmt.toml file because when I tried to format the file calling my formatter it didn't follow at all the existing style in the file. To help possible future contributors, I added a small config so that the 2-space width is preserved.

Note: I didn't test the FFI changes Since I'm just adding functions, I don't think it's too bad if something broken is pushed, but it's a bit hard to setup everything to test it in Emacs; so I'll just be confident that the simple functions here are correct, given that Emacs crate API isn't too bad.

We probably still want proper testing, so if @justinbarclay you have any "parinfer-rust dev testing" tips I'm all ears

Fixes #139

gagbo avatar Aug 24 '23 22:08 gagbo

I just noticed now that part of the changes are also included in #133 which is going to be shorter to review, but will still fail to take non-default options into account (so it will keep parinfer-rust-mode limited to exactly situations where default options are acceptable)

gagbo avatar Aug 24 '23 22:08 gagbo

Hi @gagbo, thanks for calling this to my attention. I don't have much time to test this myself. However, I have some simple integration tests defined here that could probably be used to validate that the changes here don't break anything.

I run them using cask exec ert-runner test/**.el --quiet (as seen in the make file) If you use nix I think nix-shell -p cmake cask emacs which python3 git curl will get you most of the way there for dev dependencies.

justinbarclay avatar Aug 31 '23 06:08 justinbarclay

The issue with nix here is that it pins a very old, unmaintained version of Rust, so it might not compile with the change to use saturating_add_signed here (and other than using newly-stable API, there are a bunch of security updates that are just ignored by using an old toolchain like this). I'll try to install cask externally then.

gagbo avatar Aug 31 '23 08:08 gagbo

If this takes a while to get merged, and if it doesn't have breaking changes, I'd be happy to add it to my fork as some form of beta release.

I'd love it if it was easier for users to better support their lisp of choice. Which, if I understand correctly, this would allow due to being able to configure the options object

justinbarclay avatar Sep 13 '23 18:09 justinbarclay

Yeah, sorry I forgot to rebase the PR on that, but indeed I tried to make all the knobs accessible, so that at least we can configure parinfer as well from lisp.

But the PR is also going to be blocked by nix not using a recent version of Rust, and I don't have the courage to delve into that yet

gagbo avatar Sep 13 '23 18:09 gagbo

But the PR is also going to be blocked by nix not using a recent version of Rust, and I don't have the courage to delve into that yet

yeah I tried update nixpkgs to 23.05 on my fork but neovim and vim tests were failing on me and i have no clue how to approach those, so I have to leave that work to more capable hands.

justinbarclay avatar Sep 18 '23 01:09 justinbarclay

Hey, @gagbo this has been sitting here for awhile. So, I'll just say again that I'd be happy if you opened a PR for this on my fork, https://github.com/justinbarclay/parinfer-rust, so we can work on getting this into parinfer-rust-mode.

justinbarclay avatar Mar 18 '24 02:03 justinbarclay

Yeah makes sense. I’ll need some time to figure things out and remember where I was with this, but I’ll try to make a PR to your fork this week

gagbo avatar Mar 18 '24 15:03 gagbo