-
Notifications
You must be signed in to change notification settings - Fork 162
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
Modify package info to return remote path #1100
Open
dapritchard
wants to merge
21
commits into
emacs-ess:master
Choose a base branch
from
dapritchard:modify-package-info-to-return-remote-path
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Modify package info to return remote path #1100
dapritchard
wants to merge
21
commits into
emacs-ess:master
from
dapritchard:modify-package-info-to-return-remote-path
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Updates `ess-r-package-use-dir` and `ess-r-package-eval-linewise`.
The new name is `ess-test-create-remote-path`.
This is because we are appending an element to a Tramp variable in the file.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1057.
As noted in #1057, the main source of the problem is that
ess-r-package-info
was returning a local path, even when the package was located on a remote system and hence should be addressed using a Tramp filename. I am proposing in this PR thatess-r-package-info
should return the remote path and let functions be responsible for converting the remote path into a local path if the local path is what is needed (for example, when providing a path as in input for an R process).This change does affects a fair amount functions that make use of
ess-r-package-info
, and some functions had to be modified to continue to work properly after the change. In order to provide some assurance that nothing would get broken by this change, I tried to add some tests for most of the effected functions. In particular, I borrowed some code from the Tramp tests that creates a mock remote file protocol that can be used to test functions with files on remote systems.The following are some notes that I created for myself to track what functions where effected by the change to
ess-r-package-info
, and whether anything and/or what needed to be done to incorporate the change.Functions and variables that are effected by the change to
ess-r-package-info
ess-r-package--info-cache
. This results in the variable being changed sinceit is used to cache the result of
ess-r-package-info
.ess-r-package-project
. Changes the behavior of the function since it returnsa list with one of the elements being the remote project path rather than a
local path. A test was added for this function.
ess-r-package-name
. Doesn’t affect the function since the:name
part ofthe plist doesn’t change.
ess-r-package-source-dirs
. The entire reason for changingess-r-package-info
is that it fixes a bug in this function. A test was addedfor this function.
ess-r-package-use-dir
. The internals of this function are updated with thenet result of there being no change to the behavior of the function. A test
was added for this function.
ess-r-package-enable-namespaced-evaluation
. This shouldn’t change thebehavior of this function since it just checks for a non-nil value of the
:root
portion of the(ess-r-package-info)
plist.ess-r-package-eval-linewise
. The internals of this function are updated withthe net result of there being no change to the behavior of the function. A
test was added for this function.
ess-r-package-auto-activate
. Doesn’t affect the function since it justchecks for a non-nil value of
(ess-r-package-info)
.So in summary,
ess-r-package--info-cache
,ess-r-package-project
, andess-r-package-source-dirs
are effected by the change. Next, each of thesefunctions or variables are examined for their effect on other functions.
Functions that make use of
ess-r-package--info-cache
ess-r-package-info
. The change iness-r-package--info-cache
is requiredfor the correct behavior of the function. A test was added for this function.
ess-project-save-buffers
. The change iness-r-package-info
should actuallyhave a side benefit of fixing the behavior of this function on remote
machines. A test was added for this function.
ess-r-package-re-activate
. This function isn’t effected since the functionsimply overwrites the current value of
ess-r-package--info-cache
.Functions that make use of
ess-r-package-project
ess-r-package-mode
. The functioness-r-package-project
is added to thehook
project-find-functions
.ess-roxy-enable-in-cpp
. Doesn’t affect this function since it just checks fora non-nil value returned by
ess-r-package-project
.inferior-ess-r--adjust-startup-directory
. This update provides the correctbehavior since it’s comparing filenames based on the result of
ess-r-package-project
to the value of=default-directory
, the latter ofwhich has a remote file path when appropriate. A test was added for this
function.
ess-r--find-lintr-file
. This change should provide the correct behaviorsince it will now look for a
.lintr
file using a remote path when on aremote machine. A test was added for this function.
Functions that make use of
ess-r-package-source-dirs
ess--dbg-find-buffer
. This is the function that calls the faulty version ofess-r-package-source-dirs
, so this should be fixed now.