Closed wurli closed 2 months ago
From your description, everything seems good to me, but perhaps the first three options could be only two:
config.pipe = "native"
(default, with "no" and "magrittr" as other valid options)config.pipe_map = "<LocalLeader>m"
I agree the interface could (should?) be simplified. After a bit more thought, I think my preference would be:
Use config.pipe_type = "auto"
(or "native"
or "magrittr
"). The auto
option would set the magrittr pipe if the current R version is < 4.1, the native pipe otherwise. All 3 options would have lower priority than the .Rprofile
setting if we also had config.rproj_prioritise = { "pipe_type" }
.
auto
option is that it might make the overall interface too complicated... If you think so then maybe just native
and magrittr
.Use config.pipe_map = nil
to disable the pipe mapping
Supersede config.assign = false
in favour of config.assign_map = nil
to make the interface uniform
I agree with the auto
option. But the nil
value doesn't work for the function validate_user_opts()
because both an option with value nil
and an inexistent option give the same result (false
) and, then, the warning "Unrecognized option". That's why no option has nil
as its default value. We only have nil
as the value of hook
members.
I see - thanks for explaining. How would you feel about using another sentinel to indicate no mapping, e.g. {}
or a special string like "none"
?
I think my aversion to combining config.pipe
and config.use_native_pipe
is that the .Rproj
setting should have higher precedence than use_native_pipe
, but lower than pipe
, so IMO it would make it hard to understand how the config.rproj_prioritise
option is interacting with the other settings.
What about the options for config.pipe
being "Rproj" (default), "native", (default if there is no .Rproj
in the current directory or the directory above), "no" and "magrittr"?
To be honest it still doesn't feel like the optimum solution to me 🤔 E.g. it then wouldn't be possible to then have magrittr as default, possibly overridden by .Rproj
. Plus, if support for more .Rproj
fields is added, I think it'd be nice to keep them all configured in the same place so users can quickly see how R.nvim
behaviour might vary between projects.
I think I still find more options to clearer en balance. Maybe it'd be more palatable to lean into subtables:
config = {
-- ...
assign = {
create_keymap = true,
keymap = "<M-->"
},
pipe = {
create_keymap = true,
keymap = "<localleader>m",
version = "auto",
-- A .Rproj option could go here, or (I think my preference) we
-- could collect them all together in `config.rproj_prioritise`
-- prioritise_rproj = true
},
-- ...
}
Side note: I used 'keymap' instead of 'mapping' here because I think it could be considered a little clearer given the naming scheme used elsewhere, e.g. vim.keymap.set()
, vim.api.nvim_set_keymap()
.
t then wouldn't be possible to then have magrittr as default, possibly overridden by .Rproj
You are right!
I like the pipe
option as a table. That way, it's a single option.
@jalvesaq I think this is ready for review now 😃
Still only adds support for the pipe operator, but as it's already a fairly big PR I thought best to add any extra .Rproj
-affected features later.
One observation before trying the code: you call require("r.config").rproj_setup()
in all of our 5 filetypes right after real_setup()
, so, perhaps the function could instead be called just once within real_setup()
.
Note: I never worked with .Rproj
, so I will mostly look if there are any new bugs in old features.
Nice refactor of validate_user_opts()
as apply_user_opts()
!
I noted only one mistake: in valid_types
you named external_term
as external_type
.
Did you try in your init.lua
different values for 'autochdir' and, then, start nvim
in one directory and edit an R file that is in another directory? Will the .Rproj
file be found?
vim.o.autochdir = true
vim.o.autochdir = false
And what about different values for the R.nvim
config option setwd
?
As I said, I never worked with .Rproj
, but if RStudio looks for a .Rproj
file in the parent directories, users will soon request that R.nvim
do the same.
Looking at the changes that you have made to assign
, I realized that there is no need to use two options. There is no need to have assign
(true
or false
). If assign_keymap
is ""
, then, don't create the map at all. I can make this change after your pull request is merged to avoid merge conflicts.
In two or three hours from now, I'll try to push some commits into your pull request...
Thanks! Sorry for the late response - been a bit busy since Sunday but done a little work since then in light of your comments. Let me try and push those in the next hour or two, I'll let you know once done so you can go ahead with any edits you want to make.
Did you try in your
init.lua
different values for 'autochdir' and, then, startnvim
in one directory and edit an R file that is in another directory? Will the.Rproj
file be found?vim.o.autochdir = true vim.o.autochdir = false
And what about different values for the
R.nvim
config optionsetwd
?As I said, I never worked with
.Rproj
, but if RStudio looks for a.Rproj
file in the parent directories, users will soon request thatR.nvim
do the same.
This is a good point. RStudio will ignore .Rproj
files unless you tell RStudio to open to that project - i.e. to directory where the .Rproj
file exists (most users would usually open RStudio by double-clicking the .Rproj
file). In other words, .Rproj
settings will only be applied if you start the session in a working directory where a .Rproj
file exists.
That said, I think searching upwards would likely only make life easier for people, so happy to add that in anyway.
Hi @jalvesaq, please go ahead with any extra commits you want to make 👍 If you revert assign.keymap
and assign.create_keymap
to just assign_keymap
I might also do similar with the pipe
subtable.
Incidentally, could we rename assign_keymap
to assignment_keymap
? I keep reading 'assign' as a verb - I think the noun form is a bit clearer.
Incidentally, could we rename assign_keymap to assignment_keymap? I keep reading 'assign' as a verb - I think the noun form is a bit clearer.
Yes, we can.
Done! Now there is a single option related to the assignment operator: assignment_keymap
.
@jalvesaq FYI I'm still working on this, just quit busy currently, but would like to get it right before merging. Will keep you posted 😃
@PMassicotte noted that <LocalLeader>m
is the key binding for sending lines in a Vim motion (https://github.com/jalvesaq/zotcite/issues/77#issuecomment-2066323907).
Another problem: most frequently, the pipe operator is followed by a line break, but, if using the key binding to insert the pipe operator, we will get a trailing space in the pipe operator's line after pressing the <Enter>
key.
I've made a few changes:
The .Rproj
file is now searched for on a 'per-buffer' basis, and options are recorded using nvim_buf_set_var()
. This means that we can have the pipe keymap insert |>
for one script, and %>%
for another, as dictated by the corresponding .Rproj
files. autochdir
has no effect on the behaviour.
I've changed the default pipe keymap to <localleader>,
to avoid the conflict with sending lines in a motion.
Regarding the trailing spaces, I previously wasn't too bothered about this. As far as I'm aware, very few R users care about trailing whitespace - although {lintr} will complain about it. If we want to get rid of it, some options are:
Remove the trailing whitespace from the insertion, e.g. insert |>
instead of |>
. I'd prefer to avoid this option - I think the second version with trailing whitespace is the lesser of two evils when compared to a mapping which doesn't insert the right amount of whitespace for inline pipes.
Set up an autocommand to delete trailing whitespace if a newline is inserted right after a pipe. I had a crack at this using nvim_create_autocmd("InsertCharPre", ...)
, but I couldn't get it working since this doesn't trigger on carriage return ☹️
Any thoughts?
@jalvesaq all look okay now? I found a way to strip trailing whitespace if a new line is inserted directly after a pipe, basically by temporarily remapping <CR>
and <C-j>
after a pipe is inserted, and removing the keymappings if anything but <CR>
or <C-j>
is entered after the pipe.
I updated the keybinding in the README to <LocalLeader>,
and added the documentation to doc/R.nvim.txt
(I copied some text from what you have written in the README and in lua/r/rproj.lua
). I guess everything is OK now.
Thank you! 🙏
Addresses #108.
This PR is somewhat a work in progress with fairly minimal testing - for now I'm really just looking for a sense check on the approach I've taken.
New features
This adds a mapping for the pipe which may either insert
|>
or%>%
, depending on whether an.Rproj
file is present and contains the fieldUseNativePipeOperator
. The.Rproj
-awareness can be configured usingconfig.rproj_prioritise
; for the pipe operator it is on by default.All changes
The filetype for
.Rproj
has been changed todcf
(wasyaml
)New
utils.read_dcf()
to read.Rproj
filesNew module
r.rproj
for finding/parsing.Rproj
files:apply_settings({config} [, {force}, {file}])
updates theconfig
table using a.Rproj
file if one is present. This respectsconfig.rproj_prioritise
.parse([{file}])
finds a.Rproj
file if one is present in the current working directory, and parses it into a dictionary of name/value pairs. NB, I considered recursively searching upwards through the directory tree to find a.Rproj
file but decided against it.New
config.rproj_setup([{force}, {file}])
for applying.Rproj
configuration to the current session. I was a bit torn about where this function should live, but theconfig
module seemed most appropriate given how existing code is structured.New config options:
config.pipe = true
config.pipe_map = "<localleader>m"
use_native_pipe = true
config.rproj_prioritise = { "use_native_pipe" }