Improve devtools integration #282
Conversation
|
Hi Lionel, All these looks truly great. Unfortunately I am just starting my very long trip to Asia. I will be able to have a closer look at it in a couple of days only. Same holds for your other patch (#281). (But I can also look at them even you have merged them.) Is the plan to keep all shortcuts in dev-map? I would like that but I am afraid we don't have that much free space there. We will need a lot of devtools related keys. So maybe keep everything in one devtools map? Where to hook it then? |
|
Don't worry Vitalie. I will hold those PR here until the new patch release has come out. I'm not really qualified to come up with good Emacs-style bindings because as an Evil user I don't use them. I wish you a very nice trip! |
lionel-
force-pushed
the
dev-path
branch
2 times, most recently
from
612d4f3 to
f5196f2
Compare
Useful when using a scratch file outside the R package directory.
Don't ask for package path if the selected package is the same as the one containing the current file.
* More loading options * Function to perform checks on the package * Function to document the package `ess-developer-load-package-command' is now obsolete. Custom commands can be send through (ess-developer-send-process) instead.
Previously, only "R" directories were detected. Now, a whole ranges of directories are detected. The challenge is to avoid recursive algorithms because they are slow in remote environments. The new system uses a new variable `ess-developer-package-dirs` which contains an alist of directory names and their depth in the package hierarchy. For example the "testthat" folder is usually at depth 2 compared to the package root. This can be user-customised.
Check if `ess-developer-local-package` is set in the attached process buffer, in which case use it. This way we can use devtools with last used package in files that are not connected to a package.
|
@vspinu I wonder if the whole
This allows ESS to select the right package pretty much all the time. By comparison, maintaining a list of known packages and switching manually between them seems cumbersome. Should we deprecate this? |
|
Would |
I have started working on improving the workflow some time ago (devUI branch). And it goes in line with your desire. But ess-developer-packages` is still needed if you want to source into a foreign package on the fly. You will need to add a package into the development list and activate ess-developer. I often do that with core packages or packages which I don't want to clone.
If possible, we need to strive for the same behavior on remotes and locals. Why is this needed? For
Not really. |
Yes, you had a note in the code that checking all folders was slow on remotes. But it'd still be nice to check all folders when working locally.
How about a |
I am not sure what's the added benefit of it? It seems less general than the current state but I a open to suggestions. If we can streamline UI and simplify the code I would be happy to have it. Just to be clear, the current situation is like follows. There is We can in principle merge those two steps in one. The user can be asked for package when the developer mode is turned on on a file which could not be associated with a package.
My idea was to make code injection automatic for in-package code, even if the package is not listed in |
|
I have looked through the code. Looks good. Could you please merge it in? Do you have other ideas on the short cut? We can in principle try to fit all devtools commands in C-c C-t. It will be hard but it's not impossible. |
| (ess-developer-send-process "devtools::unload('%s')\n" | ||
| "Unloading %s")) | ||
|
|
||
| (defun ess-developer-check-package () |
There was a problem hiding this comment.
How about having a prompt with completion on C-u? It would be nice to have various options for check: with/out manuals, vignetes, CRAN etc.
There was a problem hiding this comment.
Hmm... test() (normal) and the full check() (C-u) are the most common operations. Maybe have the prompt on C-u C-u? And then have check_revdep() on another keybinding.
There was a problem hiding this comment.
Or have three functions, test, check (prompt on C-u) and check_revdep, and we bind those on a dedicated keymap r-devtools-check-map.
There was a problem hiding this comment.
My common operation is to check without the vignette. I poropose that we have an allist of possible check commands with the default being the first one in the list. It's one extra RET after C-u but it saves us a lot of trouble.
A major benefit is that users can add their own check commands with whatever additional build args they need.
It might be good to extract all devtools commands into user vars for UI consistency.
There was a problem hiding this comment.
It might be good to extract all devtools commands into user vars for UI consistency.
On the other hand it's easy enough for a user to bind her own functions wrapping (ess-developer-send-process). Is it really necessary to complicate the code/UI?
There was a problem hiding this comment.
Valid point, but that would also involve binding to a key. Let's see what we end up with. It's more about the consistent code than UI, IMO. It's not good to leave some commands with custom vars and others without.
There was a problem hiding this comment.
How about the following consistent scheme:
- C-u → alternative command
- C-u C-u → main command with custom args
There was a problem hiding this comment.
so for checking:
- nil →
check() - C-u →
check(vignettes = FALSE) - C-u C-u →
check()with prompt
For loading:
- nil →
load() - C-u →
load(recompile = TRUE) - C-u C-u →
load()with prompt
There was a problem hiding this comment.
I actually meant:
- nil → load()
- C-u → load() with prompt
We place alternative command as the default in the list (press RET and get it). Otherwise the system becomes too complex. User has to remember all these C-u things and the alternative command. We would also need to add configuration both for the alternative (C-u) and the list (C-u C-u) of commands. Not all users care about Vignetes.
For one RET speedup it's not worth it.
There was a problem hiding this comment.
One problem is that the shortcuts will be too long: C-u C-c C-t C-t c. I am always forgetting C-u and typing the command twice. We can have the with prompt command on the shifted last key C-c C-t C-t C, so that when you get to the last key you will decide if you want the default "check" or prompted "check".
I use my own shortcuts with |
Yes I think just setting the package in the file-local variable + smart package detection should be enough. Then we can simplify the UI and the code. But it's entirely your call :)
Yup that's what I also think. |
|
One option is to hook the whole devtools on C-c M-t or C-c M-d. Or obsolete C-c C-t C-t as we discussed above. This one as it's very fast to type.
So you rebind keys for all modes you use? Always wanted to ask, have you switched from VIM to emacs? |
|
BTW, developer mode is not really a mode, it's just an entry and exit hook which users can customize. It might be worth to make it a proper minor mode. |
Ok, I'll restore it as a minor mode. Sorry for the confusion.
But that means that injecting code to |
|
Or we just have two commands, one for injecting to current env, the other for injecting to current package? This would work with a unique package variable while avoiding the problem I just mentioned. |
It wasm't a minor mode before. No need to restore. Let's think what's the best and most concise way to achieve what we want.
Right. That's a problem.
I actually was thinking to extend this functionality and to allow sourcing code into any visible user defined environment. Maybe having two variables is indeed unavoidable. It will be hard to display both current dev package and current environment in the modeline. Ok. Let sleep on these things. I started going through the code and making small changes here and there. Are you working on the right now? Would be good not to clash. |
|
BTW, with new names the mnemonics for shortcuts are lost. It feels awkward quite frankly. |
No, I have a deadline tomorrow, and will be on vacation for a week after that.
You mean for C-c C-t C-a / C-c ? |
Ah. Ok. Then go to work. Don't bother with this. It can wait.
Yes. Also C-c C-t C-t xx is mind blowing. I find myself forffgetting how many times I pressed C-t and already confusing C-t and C-t C-t map. I think it's a bad key :( |
|
well it was your suggestions ;) I think of C-a as "Adding" a package to the file-local vars section. But yes C-c is good for injecting to current but not for package. I had these on
If we don't find a better key, we should at least suggest in the documentation that devtools users should bind the keymap to
Aye :) |
Yeh, and it sucks. My hope was that it won't surprise existing users. But now it looks to me that the functionality is so different that this argument doesn't hold anymore. BTW, why do we need to set the package once again? I think we can do just fine without. Then have one command to set an injection environment in a broad sense (either a package or current environment or user defined environment). |
It's for files outside a package. I often have a scratch file for working on packages and so it's nice to have devtools commands always work correctly while I'm visiting that file. So C-c C-t C-a adds the information to the file-local section of that file. But it should be a rarely used command. |
|
For this use pattern we can do without the second command. You can select the source environment for that buffer. If environment is a package, we activate the developer mode there. Problem solved. We will need a real developer mode in order to automatically activate devtools and some parts of ESS functionality in C/RCpp and other relevant files. |
Hmm I'm not sure I get it. The point is to set what package should be used for a given file or directory once and for all. |
|
Btw, I have other good stuff that I will integrate in the future, like automatic detection of R process for gud debugging. |
If you are developing from external file you are likely to want to source in the
What would it bring over the tracebug? You cannot go much further existing It's not really a visual debugger, but I don't know much about it. It's not well |
I didn't know that. Nice!
I still think that such side-effecty functions should be optional. One user var does not cost a lot. In my current workflow I rely entirely on devtools —
I meant for debugging compiled C/C++ code ;) |
Indeed. But one more confusing comand does. And I am not sure if your use case is worth it. It doesn't make much sense to use One easy solution would be to prompt for package in |
It makes sense that, when you trigger load_all() or any devtool command in a file belonging to a package, it uses automatically the relevant package path. I use those devtools commands like 50 times a day (if not a 100), I don't want to enter RET each time when there is no need... But I'm not sure I understand what you're saying concretely. If I open a package file, I'm developing that package indeed.
Specifying one buffer-local var does not make the UI dirty. Minimalism has its virtues but at the same time Emacs needs to be flexible and do the right action with minimal hassle. |
I was not proposing that.
I was reffering to your corner case example when you said you want to have devtools in a file which doesn't belong to a package. I said that's rare and there is no point to have a separate command to set some local var just for that corner case. |
In my workflow it's not a corner case. I have one such file (sometimes several) for every package I develop / contribute to. It's not a big deal not to have the command, but it's not a big deal providing it either. Maybe just don't provide a keybinding for it? |
|
After sleeping on it a bit, I think you are right. We won't be able to isolate these two things. They are quite distinct conceptually, and accessing devtools need not even be from the R sources. This is how I am seeing it right now. First we need a minor mode I propse to rename it to The package selection command could be named What do you think? |
|
This sounds good, especially binding the map within the minor mode. We'll have the map activated inside Rcpp files, which is a huge improvement. Where would we bind the devtools map? Otherwise this all sounds great. |
All devtools will be on C-c C-w directly. They operate per package not per file. What do you mean by "devtools command are R only"? If you are concerned about non R dialects in ESS, then we can call it
They are a lot because you needlessly made two commands for each task. Once we have one command per task, we will be fine. We have full map available; it should be doable with good mnemonics. I think there will be very few non devtools commands on that map. |
I doubled the commands on your request because you didn't like to use C-u ....
Or |
I think it was a miscommunication. I proposed to put optional forms of the command on C-u. C-u should ask for alternative arguments with completion. Not like now, when only one alternative is executed.
A bit of an inconvenience is that we will have ess-dev-map and |
No, that's how it worked before. Since the map prefix was a bit long (C-c C-t C-t), you found it inconvenient to have alternative prompts on C-u.
I like these. |
|
Maybe the shorter |
|
|
|
I think |
I prefer your former suggestion then, |
I thought to keep ess-r-devtools prefix for functions even if they reside in package-dev map. We already have |
This improves devtools integration in several ways.
Additional functions:
C-c C-t c. With no prefix, run tests. With single prefix, run R CMD check. With double prefix, check only documentationAll these commands run through
(ess-developer-send-process). I thus deprecated the variableess-developer-load-package-commandsince users can use the function to send any custom commands.Keeping track of packages
A new buffer local variable
ess-developer-local-packagecan be set to a cons cell with package info:(name . path). This way we can use devtools commands without activating developer mode. It also works in files outside a package hierarchy.Furthermore, using a devtools command will set this local variable in the inferior buffer as well. This way the commands work when called from that buffer and it is useful to create functions using this information. For example I use it to reload R and the current package automatically (useful when debugging a crashing function).
Smarter package selection
This one is not related to devtools. When adding a package, the user is no longer prompted for the path when the selected package is the same as the package containing the current file.
In addition, I'd like to never ask for the path when we call a command from a package file. Would there be objections for this?