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

runSelection submits proceeding comments #1244

Closed
gowerc opened this issue Oct 30, 2022 · 6 comments
Closed

runSelection submits proceeding comments #1244

gowerc opened this issue Oct 30, 2022 · 6 comments
Labels

Comments

@gowerc
Copy link
Contributor

gowerc commented Oct 30, 2022

Am wanting to re-open #438 as I was hoping to work on developing a PR for it.

As far as I can see it there are 2 options to solve this

(1) Add a pre-processor that strips comments from the selected range
(2) Adjust expandSelection to not cover these lines

(1) is easier to implement but am thinking (2) is probably the more "proper" way of handling this. @renkun-ken any preference as to how to handle this ?

@gowerc gowerc added the bug label Oct 30, 2022
@gowerc
Copy link
Contributor Author

gowerc commented Oct 30, 2022

My gut feeling is expandSelection() is very complex and potentially quite fragile to changes. Currently white space is removed in post processing by the trim() method:

selection.selectedText = currentDocument.getText(selection.range).trim();

So would propose do post processing to remove header comments as well

@renkun-ken
Copy link
Member

In a previous issue or PR (I forget which one), I find it useful to keep comments in the terminals so that when I scroll the terminal I could also read the comment to know about the code just like in the source code. But if the comment block has too many lines, it might be a bit annoying.

Maybe we could let user to opt-out removing leading comments if they find it annoying?

@gowerc
Copy link
Contributor Author

gowerc commented Oct 31, 2022

Makes sense will add an argument flag to enable/disable the feature. Any preferences on default? Personally I would argue that we should mirror rstudio which would be to have this enabled by default.

Likewise any thoughts on wether this should be for expandselection() only or for any submission to terminal ? I imagine the latter but I think I would need to re-do the implementation and it might also be more complicated :(

@renkun-ken
Copy link
Member

The default behavior of RStudio is keeping the leading comments. I suggest we keep it as default too. User could opt-out the behavior by a setting.

I think expandselection() having this behavior is enough.

@gowerc
Copy link
Contributor Author

gowerc commented Nov 23, 2022

Agreed, I will rework so that it only applies to expandselection().

The Rstudio implementation seems to be space dependent i.e.

# l1
# l2
x <- c(
    2,
    3,
    4
)

Would include the comments whilst:

# l1
# l2

x <- c(
    2,
    3,
    4
)

Would not include the comments.... I don't think we should replicate this exactly, though it confuses me as to what our default should be.

renkun-ken added a commit that referenced this issue Feb 1, 2023
* Remove leading comments from terminal submission (#1244)

* converted in -> of to appease lintr

* updates to appease lintr

* added unit test for double comment line

* Add r.removeLeadingComments to settings

---------

Co-authored-by: Kun Ren <renkun@outlook.com>
@renkun-ken
Copy link
Member

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants