Skip to content

Change so setting changes take effect immediately#301

Merged
Ikuyadeu merged 5 commits intoREditorSupport:masterfrom
andycraig:change-settings
May 2, 2020
Merged

Change so setting changes take effect immediately#301
Ikuyadeu merged 5 commits intoREditorSupport:masterfrom
andycraig:change-settings

Conversation

@andycraig
Copy link
Copy Markdown
Collaborator

Closes #299

What problem did you solve?

Currently, changing a vscode-R setting requires a reload of the extension before the setting change takes effect. This PR makes setting changes take effect immediately.

How can I check this pull request?

Create a file temp.R with content

echo "Hello World"

Enable setting R: Always Use Active Terminal.

Open a terminal. Do NOT start R.

Ctrl+Enter in temp.R.

Observe that Hello World is sent to the terminal.

Disable setting R: Always Use Active Terminal.

Ctrl+Enter in temp.R.

Observe that a new R session starts in the terminal (with an error because echo is not valid R code).

Previously, R: Always Use Active Terminal would not have taken effect until the extension was reloaded (e.g., by restarting VS Code).

@andycraig
Copy link
Copy Markdown
Collaborator Author

I committed with unsaved changes to session.ts. I will force-push the changes to that.

Comment thread src/extension.ts Outdated
Comment thread package.json Outdated
@renkun-ken
Copy link
Copy Markdown
Member

Another question is that it seems the config variable in utils.R (https://github.com/Ikuyadeu/vscode-R/pull/301/files#diff-8cd4968b81985f0efc2053eabc37db6dR7) seems no longer used any more. Should we clean it up and the imports?

@andycraig
Copy link
Copy Markdown
Collaborator Author

@renkun-ken Good catch! Will do.

@andycraig
Copy link
Copy Markdown
Collaborator Author

Hmm, we could retain the intention of config by changing it to a function instead of a variable.

Copy link
Copy Markdown
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread src/util.ts
import path = require('path');
import { window, workspace } from 'vscode';
import winreg = require('winreg');
export let config = workspace.getConfiguration('r');
Copy link
Copy Markdown
Member

@Ikuyadeu Ikuyadeu May 2, 2020

Choose a reason for hiding this comment

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

how is just change to

export const config = (function() {
    return workspace.getConfiguration('r');   
});

It will reduce whole of changes.
by calling

- config.get("")
- workspace.getConfiguration('r').get("")
+ config().get("")

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Ikuyadeu Exactly, that's what I was thinking of.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Ikuyadeu

export const config = (function() {
    return workspace.getConfiguration('r');   
});

is better than

export function config() {
    return workspace.getConfiguration('r');
}

?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes this looks better.

Copy link
Copy Markdown
Member

@Ikuyadeu Ikuyadeu May 2, 2020

Choose a reason for hiding this comment

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

I agree with you. My example was misleading between with function and variable

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I will change to this.

export function config() {
    return workspace.getConfiguration('r');
}

The commits are becoming a bit messy. I would like to tidy up using REBASE and FORCE PUSH. @Ikuyadeu @renkun-ken Do you approve rebase and force push here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this case, Squash and Merge is better, it will combine your commits to the one.
After your change, I will merge it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Ikuyadeu Okay, no rebase. Thanks in advance for squash and merge.

I will add the commit for config().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm totally fine with both.

@andycraig
Copy link
Copy Markdown
Collaborator Author

Diff is much smaller now.

@Ikuyadeu
Copy link
Copy Markdown
Member

Ikuyadeu commented May 2, 2020

Great!

@Ikuyadeu Ikuyadeu merged commit 2185651 into REditorSupport:master May 2, 2020
@andycraig
Copy link
Copy Markdown
Collaborator Author

@Ikuyadeu @renkun-ken Thanks both!

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.

Have setting changes apply immediately

3 participants