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.

Sign up for GitHub

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

Jump to bottom

Emacs respect string delimiter default #142

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Emacs respect string delimiter default #142

wants to merge 10 commits into from

Conversation

gagbo
Copy link

@gagbo gagbo commented Aug 24, 2023

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
Copy link
Author

gagbo commented Aug 24, 2023

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)

@justinbarclay
Copy link
Contributor

justinbarclay commented Aug 31, 2023 •
edited
Loading

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.

@gagbo
Copy link
Author

gagbo commented Aug 31, 2023

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.

@justinbarclay
Copy link
Contributor

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

@gagbo
Copy link
Author

gagbo commented Sep 13, 2023

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 gagbo force-pushed the emacs_respect_string_delimiter_default branch from 2f6582d to 89b878f Compare September 14, 2023 07:56
@justinbarclay
Copy link
Contributor

justinbarclay commented Sep 18, 2023 •
edited
Loading

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 and others added 3 commits March 17, 2024 17:07
@justinbarclay
Add guard when converting from i64 to usize
3584c57 
In some cases, when bad data is passed in as a set of change to
parinfer-rust it can cause parinfer-rust to crash. This crash is
caused by trying to add indents to a line. When the delta calculated
turns out to be negative and when that is added to orig_indent that
can cause issues when type casting to usize (Column). This is because
the combination of these two variables can be negative and when type
cast to a usize it causes the number to be very large, in my case
something like 1.8 trillion. So when this number is then used to
determine how many times to repeat a string, it causes the library to
run out of memory and crash.
@justinbarclay
Update actions to support macos arm builds
85a6a80 
Update release script
@justinbarclay
Merge pull request eraserhd#5 from justinbarclay/jb/flake (do not mov…
9c287bb 
…e to upstream)

Update to use flakes and latest version of nixpkgs

Do not move upstream due to this commit breaking integration tests
@justinbarclay
Copy link
Contributor

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.

@gagbo
Copy link
Author

gagbo commented Mar 18, 2024

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 gagbo force-pushed the emacs_respect_string_delimiter_default branch from 89b878f to ed7d868 Compare March 18, 2024 20:59
justinbarclay and others added 5 commits March 18, 2024 21:02
@justinbarclay
Add emacs to default features (do not move to upstream)
ed1fabd 
For ease of development make sure that the emacs features are always
tested against
@gagbo
[Emacs] Use default CLI options when nothing is specified
5f99e91
@gagbo
[Emacs] expose setters and getters for request options
b39d480
@gagbo
lint: add rustfmt.toml and pass format
3cb800a 
Using the configuration `tab_spaces = 2` to minimize the impact on
formatting src/emacs_wrapper.rs
@gagbo
Backport justinbarclay/parinfer-rust-emacs@67924eefef
3d328cd 
This is redone by using `saturating_add_signed` instead of having an
extra helper function. The function is stable since Rust 1.66
@gagbo gagbo force-pushed the emacs_respect_string_delimiter_default branch 3 times, most recently from c73800a to adb89df Compare March 20, 2024 21:43
@gagbo gagbo mentioned this pull request Mar 20, 2024
Merged
@gagbo
Part 1 of compilation fixes
180acb8 
Upgrade to emacs crate 0.19

Fix all conversions to/from Lisp except the Vec<String> <-> Vector equivalence

Apparently one reference to env keeps escaping
@gagbo gagbo force-pushed the emacs_respect_string_delimiter_default branch from adb89df to 180acb8 Compare March 21, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parinfer Does Not Ignore Parens in String Literals [Spacemacs/Emacs]
2 participants
@gagbo @justinbarclay