Improve handling of starting directory #457

lionel- · 2017-10-07T12:37:19Z

Integrates ess-r-package-mode with Emacs 25.1 projects.
The default prompt directory is now the current project directory. This includes R packages and version controlled projects (git, svn, ...).
Prompt is made a bit nicer
If ess-ask-for-ess-prompt is nil, switch to the default directory (so the current project if there is one)

The project stuff is optional and old Emacsen will just use current previous methods for choosing the default directory. I think eventually we should deprecate ess-directory-function in favour of Emacs projects project-find-functions.

Includes a bit of code cleanup.

vspinu · 2017-10-07T17:53:27Z

lisp/ess-inf.el

-              ;; commong to restart the process in the same buffer.
-              buf-name-str (buffer-name)
-              method    1))
+                          defdir))


Why over the sudden you decided for this? I aggregate setqs whenever I see the chance ;) Find it neater and more readable. But well, it's tiny.

I thought aggregated setq were against coding standards but after a bit of googling it seems I was mistaken and there are two standard visions of two groups of people arguing over this ;)

Personally I prefer disaggregated because it attracts attention over small infelicities (mutation), are easier to refactor, and I personally find it more readable. I can switch it back to previous form if you prefer.

Nevermind. It's not important. Not what we have some high standards in this project for coding style:). But in my code I always aggregate as I find all those side-effectful repeated setq noisy and annoying.

vspinu · 2017-10-07T18:14:11Z

lisp/ess-inf.el

+    (directory-file-name dir)))
+
+(defun inferior-ess--get-directory (procname dialect)
+  "This returns the directory of the current project"


Nitpicking, but let's do new docs according to emacs standards (imperative statement + properly document all params).

The name is not particularly suggestive. What directory does this return more concretely? Shouldn't this just be part of ess-prompt-for-directory? It's a bit strange that a helper -- function calls main utility function and not the other way around.

let's do new docs according to emacs standards

Agreed. That was a leftover from the time where this was a (def-generic ess-current-project (). I'll just remove the docstring.

The name is not particularly suggestive.

Agreed, it should probably be inferior-ess--get-startup-directory. I don't like that function either and thought about merging it in ess-prompt-for-directory but it's a bit annoying because of that dialect specific handling. Also this checks for the value ess-ask-for-ess-directory to avoid multiple check in inferior-ess-mode.

It's a bit strange that a helper -- function calls main utility function and not the other way around.

I think that's alright if it reduces repetition in another main function.

vspinu · 2017-10-07T18:20:41Z

lisp/ess-r-package.el

@@ -87,6 +87,11 @@ whether the current file is part of a package, or the value of
      (with-ess-process-buffer t
        ess-r-package-info)))

+(defun ess-r-package-project (dir)


Why do we need so many functions that do similar stuff ess-r-package-project, ess-r-package--package-dir-info, ess-r-package-info, ess-r-package--find-package-path. It's confusing. Cannot we reduce them to 1 max 2 functions?

BTW, shouldn't it be ess-r-package-project-root or ...-dir?

There's a function that does the searching + caching, one that returns the package info in ESS format, one that returns the package info in Emacs project format, ... I'm not sure we can streamline it further but a better naming scheme would make things clearer. I'll try to simplify.

shouldn't it be ess-r-package-project-root

I don't think so because it doesn't return a project root, it returns a cons cell that includes the project name.

I'll try to simplify.

Thanks!

vspinu · 2017-10-07T18:27:31Z

Looks fine to me, but I would really want to see streamlining of ess-r-package API. It's quite hairy at the moment with all those "path/dir/root/info" functions.

lionel- · 2017-10-21T11:29:34Z

@vspinu Can you please take another look? I also tweaked the names of the source dirs functions as suggested in d559b3d.

vspinu · 2017-10-24T12:46:55Z

doc/newfeat.texi

+package root as default selection. Otherwise the working directory is
+automatically set to the package root.
+
+The mechanism is a bit more general if you are using Emacs 25.1 or


Do we really need this behavior? If I start R in a subdirectory of a git project I probably want to start in that sub-directory not in the root.

This is an unlikely scenario though and I can leave with it if it indeed simplifies and modernizes the codebase.

What about "test" sub-directory?

I think ESS badly needs a more integrated notion of project and this is the start of it. For data analysis your scripts should work from the root of the project not in subdirectories.

As for the test directory, maybe we can define a notion of subprojects within R packages? Could you describe the use cases for this please? I'm not sure when this would be useful, I never switch to the test folder in my workflow. In fact switching to that folder would be disruptive in my case. If we add this feature it should be optional.

This is an unlikely scenario though

It happens all to the time in my workflow because I often switch to a test file with projectile and then open R. Or I switch to a file in /R or /src. Or /.metadata (which is a nice folder to put your package notes and scratch files because it is automatically ignored by R CMD build and you can add that folder in your global .gitignore too). I almost never open a file at the root and then open R.

the use cases for this please?

The use case is as discussed earlier. Tests are evaluated (by R CMD check) in the test sub-directory not at the root. So if I start a fresh R session in test sub-dir, it probably shouldn't be at the root. For the eval part, tests should not be evaluate within the package.

In fact switching to that folder would be disruptive in my case.

When my test fails, I jump to that particular test and try to figure it out locally. I find it hard to imagine a different workflow.

This is an unlikely scenario though

By this I meant that you open R script in a non-R-package. My earlier concern were exclusively about such case.

Tests are evaluated (by R CMD check) in the test sub-directory not at the root.

You're right, it might be a good idea to add a notion of subproject later on then. Maybe a better integration of R CMD check would be better, i.e. ideally it would run in a fresh process. Though that poses the question of simulating an installed package which is easiest with load_all() but this would impose devtools on users. I personally (almost) never execute these scripts manually because I use testthat and devtools::test().

When my test fails, I jump to that particular test and try to figure it out locally. I find it hard to imagine a different workflow.

To debug the test you typically don't need R to be in a particular folder, unless the test uses a local resource. I guess for the case of local resources (e.g. a csv file) it does make sense to be in a non-root folder. However it seems it would be hard for ESS to predict which folder because that is dependent on development tools and test scripts.

On the other hand being in a non-root folder makes it difficult to use tools provided by packages like usethis or revdepcheck.

By this I meant that you open R script in a non-R-package.

Ok. I think the notion of projects should be helpful to data analysis users. It seems to be good practice for scripts to execute relative to the project root.

I guess for the case of local resources

Precisely.

However it seems it would be hard for ESS to predict which folder because that is dependent on development tools and test scripts.

Let's just make an exception for "test" sub-directory and be done with it. I guess, we don't want to get too complicated on this. It's not an essential functionality anyways.

It seems to be good practice for scripts to execute relative to the project root.

Good point. Let's do a bit of nudging then.

vspinu · 2017-10-24T12:48:56Z

doc/newfeat.texi

+directory. This means that any git or subversion directory will be
+recognised as project root even if it is not an R package.
+
+In light of these developments, @code{ess-r-package-get-info} is


Need not be in the newfeat, I think. ess-r-package-get-info is kinda an internal function, so I think we can just silently deprecate it. Discussion of this feature got a bit too technical.

vspinu · 2017-10-24T12:54:58Z

lisp/ess-inf.el

-                                     (get-process (ess-proc-name
-                                                   ntry
-                                                   temp-dialect)))))
+           ;; Find the next non-existent process N (*R:N*)


Much nicer. I am now amazed that I could write that beast :/.

vspinu · 2017-10-24T13:01:50Z

doc/newfeat.texi

+as symbol and the package directory as string. In addition
+@code{ess-r-package-name} returns the current package name as a string.
+
+In the future (long term), we will thus deprecate


Separate into: - inferior-ess--default-directory() - inferior-ess--get-directory()

Now that R packages are registered as projects this will return their root directory as default. Eventually we might want to deprecate `ess-directory-function` in favour of projects once Emacs 25.1 becomes the oldest supported version.

`default-directory` is unset by the time the inferior is restarted

Renamed inferior-ess--get-directory() to inferior-ess--maybe-prompt-startup-directory() Renamed inferior-ess--default-directory() to inferior-ess--get-startup-directory()

@vspinu

cc @vspinu. This is currently a bit ugly, we'll need to move all the dialect-specific stuff out of `ess-inf.el` at some point.

vspinu · 2017-10-31T11:03:38Z

Maybe it's best not to introduce another variable if no one is going to change it?

Sorry. I missed this post. Have you already introduced the variable? If not, let's not do it. I thought a bit more about it and I think it's best not to do any exceptions at all, even for the test sub-dir. Consistent behavior is a good thing.

lionel- requested review from mmaechler and vspinu October 7, 2017 12:37

vspinu reviewed Oct 7, 2017

View reviewed changes

lionel- force-pushed the fix-ask-for-dir branch from c942bda to 3490d46 Compare October 21, 2017 17:59

vspinu requested changes Oct 24, 2017

View reviewed changes

lionel- added 21 commits October 29, 2017 11:36

Disaggregate setqs

1536e1a

Clean up comments

1079d1f

Refactor loop to proper form

7565e00

Alias ess-directory to ess-default-directory

db73034

Refactor ess-get-directory()

b78ad41

Separate into: - inferior-ess--default-directory() - inferior-ess--get-directory()

Reduce noise in directory prompt

2dd3d23

Integrate ess-r-package-mode with Emacs projects

3f22ff4

Restore directory silently on reloading

36dbbd2

Use ess-default-directory instead of default-directory

9907220

`default-directory` is unset by the time the inferior is restarted

Better names for directory helpers

581ed38

Renamed inferior-ess--get-directory() to inferior-ess--maybe-prompt-startup-directory() Renamed inferior-ess--default-directory() to inferior-ess--get-startup-directory()

Rename ess-default-directory to ess-startup-directory

eb46c8d

Proper defvaralias

463aeb7

Move ess--parent-dir() to ess-utils.el

115f86c

Remove wd setter in ESSR startup as it has moved to inferior startup

5a67ea5

Use ess-r-package-project() to get project info

b736d63

Rename ess-r-package-info to ess-r-package--project-cache

00a490a

Simplify package finding functions

fa59522

Add ess-r-package-name()

924ddaa

Move news item about source dirs to R features

a7277b3

Document new project features in news file

63d07c2

Emacs projects' car contains project type not project name

2924762

lionel- force-pushed the fix-ask-for-dir branch from 3490d46 to 301cfc0 Compare October 29, 2017 10:44

Remove superfluous items from newfeat

4b2e38e

lionel- force-pushed the fix-ask-for-dir branch from 301cfc0 to 4b2e38e Compare October 29, 2017 10:46

Prefer the tests directory over package roots

5a7fb1c

cc @vspinu. This is currently a bit ugly, we'll need to move all the dialect-specific stuff out of `ess-inf.el` at some point.

lionel- merged commit 2071f30 into emacs-ess:master Oct 29, 2017

lionel- deleted the fix-ask-for-dir branch October 29, 2017 11:18

lionel- mentioned this pull request Nov 2, 2017

default directory changed? #461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of starting directory #457

Improve handling of starting directory #457

lionel- commented Oct 7, 2017

vspinu Oct 7, 2017

lionel- Oct 8, 2017

vspinu Oct 8, 2017

vspinu Oct 7, 2017

lionel- Oct 8, 2017

vspinu Oct 7, 2017

lionel- Oct 8, 2017

vspinu Oct 8, 2017

vspinu commented Oct 7, 2017

lionel- commented Oct 21, 2017

vspinu Oct 24, 2017

lionel- Oct 24, 2017

vspinu Oct 24, 2017

lionel- Oct 24, 2017

vspinu Oct 24, 2017

vspinu Oct 24, 2017

vspinu Oct 24, 2017

vspinu Oct 24, 2017

vspinu commented Oct 31, 2017

Improve handling of starting directory #457

Improve handling of starting directory #457

Conversation

lionel- commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vspinu commented Oct 7, 2017

lionel- commented Oct 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vspinu commented Oct 31, 2017