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

(WIP) Changes to improve data.table completion #20

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

Conversation

matsburg
Copy link

This is where I've got to so far with the data.table completion behaviour as discussed in #18. There are two significant changes:

  • The regular expression for checking if the completion is happening inside the brackets of a data.frame (or data.table) is changed, so that completions are offered before the first comma is typed. This behaviour can be switched on and off with the ncm_r_dt setting, which is also added. I think it would be best to change this so that the class of the object is automatically detected, and this behaviour only happens when the object is a data.table. I think this should be possible - will work on it next.

  • The other objective was to get function names as completions after the first comma is typed in a data.table command. The only way I've found to do this is to set refresh = 1 in the call to complete(), so that completions are refreshed as the user types. This is somewhat unsatisfactory since it isn't toggled by the ncm_r_dt setting (though could be I think), and presumably goes against the reason why you originally chose not to enable refreshing. Maybe it negatively impacts performance? I've not noticed a slowdown yet, but have only been working with small files. Let me know if you can think of a better solution to this problem.

I've also added some test cases to demonstrate the behaviour I'm trying to get (which should currently all pass).

matsburg and others added 2 commits March 18, 2019 20:21
- Adds `ncm_r_dt` setting. When set, this changes the regular expression that's
  used to check whether the cursor is inside the brackets of a data.frame (or
  data.table). This causes completions to be offered before a comma is typed,
  which means column names can be completed in the 'i' component of a
  data.table command, which occurs before the first comma i.e. DT[i, j, by]

- Changes completion behaviour so that matches are refreshed as the user types
  more. This means that function names are now offered as completions after the
  the first comma has been typed within a data.table command.

- Adds test cases for the new data.table completion behaviour
`data.table` objects are detected automatically using the omni object
match list.

Also:

- Adds support for completion when using `data.table`s with `magrittr`
pipes, using `.` to refer to the 'parent' DT (note that as a
side-effect, ordinary `data.frame`s can be used in the same
way and completion work as expected)

- In order to get this pipe behaviour to work, the regular expression
for checking if completion is happening inside the brackets of a
`data.frame` was changed to allow dots in the name of the DF. This could
be considered a bug fix since previously completion inside brackets did
not work as expected if the DF had dots in its name.
@matsburg
Copy link
Author

matsburg commented Jun 29, 2019

I've worked on this a bit more now, the behaviour is only triggered for data.table objects and so there's no need for the ncm_r_dt option, which is removed. I've also added support for completion when using data.tables with the magrittr pipe %>%, like this:

DT %>%
  .[i, j, by]

Note that you can also do this with ordinary data.frames, i.e.

DF %>%
  .[, "column_name"]

and completion works as expected in this case too. Coincidentally, this pipe completion behaviour isn't supported by RStudio :)

When adding this pipe behaviour I noticed that the regex used for detecting if completion is happening inside the brackets of a df doesn't allow for the name of the df to contain dots (since \w includes '_' but not '.') Hence this PR also fixes this bug. But I'd be happy to split out this fix into a separate PR if it would be more appropriate.

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.

1 participant