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

Implement rustic-format-region. #258

Merged
merged 7 commits into from
Jul 19, 2021

Conversation

rbartlensky
Copy link
Contributor

This unfortunately only works with rustfmt nightly, but it's still
an interesting function to have at your disposal.

This unfortunately only works with rustfmt nightly, but it's still
an interesting function to have at your disposal.

Fixes: brotzeit#229
@brotzeit
Copy link
Owner

brotzeit commented Jul 4, 2021

Thanks for the pull request!

What do you think about using (interactive "r") ? It would then look similar to rustic-playpen.

(defun rustic-playpen (begin end)
  "Create a shareable URL for the contents of the current region,
src-block or buffer on the Rust playpen."
  (interactive "r")
  (let (data)
    (cond
     ((region-active-p)
      (setq data (buffer-substring begin end)))
...

rustic-rustfmt.el Outdated Show resolved Hide resolved
(declare-function project-root "project")
(if (version<= emacs-version "28.0")
(declare-function project-roots "project")
(declare-function project-root "project"))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it makes sense to write a simple test that only checks if this is working. This change have been a little annoying #256

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how I would test this. Do you have any ideas?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, never mind. Maybe we add something later.

@rbartlensky
Copy link
Contributor Author

@brotzeit I attempted to address all your comments, although my elisp isn't that great, so I might need another look over this.

(append (list rustic-cargo-bin "+nightly" "fmt" "--")
(rustic-compute-rustfmt-file-lines-args file
start
(+ start len)))))))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but isn't it possible to use the arguments begin and end here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware (interactive "r") gives you begin and end 😅 , but now I know. I removed the unnecessary code. However, in this particular case, rustfmt asks you for lines, while begin and end are in bytes (AFAICT).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, in this particular case, rustfmt asks you for lines, while begin and end are in bytes (AFAICT).

I didn't notice. Then maybe (interactive "r") isn't the right solution. line-number-at-pos could help here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe we could only use end...

Just do what you think makes sense ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found a good middle-ground solution. Check out the last commit.

'rustic-format-file-sentinel
:buffer buf
:command
(append (list rustic-cargo-bin "+nightly" "fmt" "--")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this feature is only available on nightly or why do you pin the toolchain ? I guess nowadays lots of people are using stable instead, so we should cover the case when there's no nightly available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it only works with rustfmt nightly. It looks like they are close to stabilizing (rust-lang/rustfmt#3397)! I'll try to handle this more gracefully

(file (buffer-file-name buf))
(r-begin)
(r-end))
(cond ((region-active-p)
Copy link
Owner

@brotzeit brotzeit Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just do something like if region is active -> format region, if not -> format buffer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@rbartlensky
Copy link
Contributor Author

rbartlensky commented Jul 10, 2021

@brotzeit thanks again for all your input. I believe I have addressed all your comments :) (let me know if you want to squash my commits or not)

@rbartlensky
Copy link
Contributor Author

@brotzeit I think this is ready

@brotzeit
Copy link
Owner

Oh, I missed this pull request, sorry...

Thanks again!

@brotzeit brotzeit merged commit 26c2e09 into brotzeit:master Jul 19, 2021
@rbartlensky rbartlensky deleted the feature/format-region branch July 19, 2021 15:25
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 this pull request may close these issues.

2 participants