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

Allows some devtools commands to be run in a compilation-mode buffer. #531

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Allows some devtools commands to be run in a compilation-mode buffer. #531

wants to merge 1 commit into from

Conversation

atheriel
Copy link
Contributor

This is a proof-of-concept implementation of the feature discussed in #528. I've only implemented check(), test(), and document() for now, and obviously the documentation could use some work as well.

How does this general approach look?

(Known issue: compilation mode buffers do not render the shell-escaped characters produced by devtools::test() correctly.)

(ess-r-package-eval-linewise
"devtools::check(%s)\n" "Checking %s" arg
'("" (read-string "Arguments: " "vignettes = FALSE"))))
(let ((actions '("" (read-string "Arguments: " "vignettes = FALSE"))))
Copy link
Member

Choose a reason for hiding this comment

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

Too much copy pasting. I would add a wrapper which would dispatch to ess-r-package-eval-linewise.

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 had a hard time coming up with a design for such a wrapper -- do you have an idea about how to do it nicely?

"Evalate R expression EXPR in a standalone `compilation-mode'
buffer."
(let* ((procname inferior-ess-r-program-name))
(ess-r-package-compilation-start
Copy link
Member

Choose a reason for hiding this comment

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

I think this extra helper is not warranted. Could be inlined 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 was thinking the helper could be used to run non-R commands in the project directory. But perhaps it's premature to build out that feature without an actual use case.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. Let's strive for minimal code for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

(ess-r-package-set-package)))
(pkg-name (ess-r-package-name))
(default-directory (cdr pkg-info)))
(compilation-start command nil
Copy link
Member

Choose a reason for hiding this comment

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

t instead of nil would result in compilation-shell-minor-mode which should solve the term signals issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this also puts it in comint-mode, which is quite a different experience than the usual compilation-mode. I don't see an obvious way to get term signal handling outside of comint-mode -- do you have any inkling of what could be done?

Copy link
Member

Choose a reason for hiding this comment

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

Why does that matter? If compilation mode is based on comint this is how it is.

This commit introduces a `ess-r-package-use-compile-buffer` flag that
allows users to specify that `check()`, `test()` and `document()`
should be run in a standalone buffer.

Signed-off-by: Aaron Jacobs <[email protected]>
@atheriel
Copy link
Contributor Author

Hey, sorry for the long silence on this. I've made a minor update to the patch, but I'm not sure how to proceed.

@lionel-
Copy link
Member

lionel- commented Aug 10, 2018

FYI I have extracted ess-r-eval-in-compile-buffer from ess-r-package-eval-in-compile-buffer:

(defun ess-r-eval-in-compile-buffer (expr)
  "Evalate R expression EXPR in a standalone `compilation-mode' buffer."
  (let* ((procname inferior-ess-r-program-name)
         (command (format "%s --slave --no-readline -e \"%s\"" procname expr)))
    (compilation-start command nil
                       (lambda (name-of-mode)
                         (concat "*" (downcase name-of-mode) "*"))
                       ess-r-error-regexp-alist)))

I use it for rendering markdown documents:

(defun my-ess-rmarkdown-render ()
  (interactive)
  (let ((cmd (format "rmarkdown::render('%s')" buffer-file-name)))
    (ess-r-eval-in-compile-buffer cmd)))

I quite like that this uses a separate process at each time rendering. I haven't tried it yet for devtools commands but I think we should explore compilation-mode further after the next release.

@vspinu
Copy link
Member

vspinu commented Aug 10, 2018

@atheriel sorry for the late reply. We will merge this after the release in September. A bit more thinking is needed here.

@atheriel
Copy link
Contributor Author

Yes, I appreciate that. I'd like to get back to it. One thing I hadn't considered (but is pertinent to my own use cases) is how to ensure that this will interoperate with ess-remote correctly.

@jabranham
Copy link
Contributor

What's the status of this? Are we implementing it for 19.04?

@lionel-
Copy link
Member

lionel- commented Apr 3, 2019

I don't think so. But I'd still like to integrate compilation buffers in ESS at some point.

@atheriel
Copy link
Contributor Author

atheriel commented Apr 3, 2019

I'm still a big fan of this approach, although there are a few outstanding issues mentioned in the thread:

(1) Rendering of escape characters.
(2) Whether to use compilation-mode or comint-mode; and
(3) How this will work with ess-remote.

As well as any actual design questions. Any thoughts on these?

@vspinu
Copy link
Member

vspinu commented Apr 11, 2019

I had some very concrete thoughts back then but didn't have time to get to it. And now I am even more busy to have a look at this.

One thing for sure, the functionality should be brought in once we have the local cache for prog + params which is also needed for restarts.

@kguidonimartins
Copy link

This would be a nice feature! Any update?

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.

5 participants