Allow Functions for :test-dir #1671

LaurenceWarne · 2021-05-30T19:13:03Z

Hi 👋, this PR is a proposed solution to #1650, achieved by allowing the :test-dir option to take a function which returns the test directory given the implementation directory.

The behaviour for the project in #1650 with the config:

(setq projectile-create-missing-test-files t)
(projectile-update-project-type
   'sbt
   :test-dir
   (lambda (file-path) (projectile-complementary-dir file-path "main" "test")))

Is that calling projectile-toggle-between-implementation-and-test from src/main/scala/bar/package.scala sticks you in src/main/scala/bar/packageSpec.scala and calling from src/test/scala/foo/package.scala sticks you in src/test/scala/foo/packageSpec.scala (regardless of either test file existing).

Calling projectile-toggle-between-implementation-and-test from either test file however will prompt a choice between the two existing package.scala files. I can PR a similar change for allowing :src-dir to take a function if you are happy with these changes.

Setting the option to a function should override existing behaviour (for example :related-files-fn) , and if the option is not a function then the behaviour should be the same as before. I've added more detailed documentation in the README about the new behaviour, and also added a comparison of when I think you should use :test-dir over related-files-fn and vice versa.

Apologies for the large PR 🙂, I'm not sure how it could be split up on top of omitting changes to src-dir.

Thanks, let me know what you think!

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (eldev test)
The new code is not generating bytecode or M-x checkdoc warnings
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the readme (if adding/changing user-visible functionality)

Thanks!

LaurenceWarne · 2021-05-30T19:13:31Z

CHANGELOG.md

 ## 2.4.0 (2021-05-27)

 ### New features

-* Add `projectile-update-project-type-function` for updating the properties of existing project types.
+* Add `projectile-update-project-type` function for updating the properties of existing project types.


Just noticed this, sorry!

No problem.

bbatsov · 2021-05-31T05:54:25Z

CHANGELOG.md

@@ -2,11 +2,13 @@

 ## master (unreleased)

+* Allow the `:test-dir` option of a project to be set to a function for more flexible test switching.


This has to go under "New features" or "Changes".

bbatsov · 2021-05-31T06:14:17Z

projectile.el

@@ -3342,13 +3351,14 @@ test file."
           "No matching source file found for project type `%s'"
           (projectile-project-type))))
    ;; find the matching test file
-    (let ((test-file (projectile-find-matching-test file-name)))
-      (if test-file
+    (if-let ((overriding-test-file (projectile--test-file-override file-name)))


Doesn't it make more sense to incorporate this in the existing test lookup function instead of creating the potentially confusing test override concept? I definitely don't like how the code reads now.

yeah, in hindsight that makes more sense.

bbatsov · 2021-05-31T06:15:02Z

projectile.el

@@ -3443,6 +3453,35 @@ Fallback to DEFAULT-VALUE for missing attributes."
        (or (string-equal prefix-name name)
            (string-equal suffix-name name))))))

+(defun projectile--complementary-file (file-path dir-fn filename-fn)
+  "Return `(DIR-FN dir)/(FILENAME-FN name)' where FILE-PATH = dir/name.


You've quoted this a symbol reference, even though it's not.

bbatsov · 2021-05-31T06:17:06Z

projectile.el

+(defun projectile--impl-to-test-dir (impl-dir-path)
+  "Return the directory path of a test whose impl file resides in IMPL-DIR-PATH.
+
+Occurrences of 'src-dir' are replaced with 'test-dir' to obtain the new directory."


The quoting references here a confusing, as those are not some references, but rather explanations of the implementation.

bbatsov · 2021-05-31T06:17:28Z

projectile.el

+    (projectile-complementary-dir impl-dir-path impl-dir test-dir)))
+
+(defun projectile-complementary-dir (dir-path string replacement)
+  "Return the 'complementary' directory of DIR-PATH by replacing STRING in DIR-PATH with REPLACEMENT."


complementary should be double-quoted here.

bbatsov · 2021-05-31T06:20:50Z

I can PR a similar change for allowing :src-dir to take a function if you are happy with these changes.

That'd be nice indeed, otherwise it'd be super weird why we have different setups for src-dir and test-dir. This can be done in a separate PR, though.

Overall the code looks good, but I'd like to try to simplify a bit the code - in particular it took me a while to understand the need for the overriding function.

LaurenceWarne · 2021-05-31T17:11:36Z

projectile.el

-          (error "No matching test file found for project type `%s'"
-                 (projectile-project-type)))))))
+    (let* ((test-file (projectile-find-matching-test file-name))
+           (test-file-or-fallback (or test-file (projectile--test-file-fallback file-name)))


projectile--test-file-fallback here does the same as projectile-create-test-file-for did previously, minus the creation of the test file which has been moved to this function.

I'm undecided whether moving projectile--test-file-fallback to projectile-find-matching-test (as a final when-let 😄) or keeping it here would make more sense. What do you think?

LaurenceWarne · 2021-05-31T17:16:54Z

That'd be nice indeed, otherwise it'd be super weird why we have different setups for src-dir and test-dir. This can be done in a separate PR, though.

Thanks, hopefully that should be straightforward once you're happy with this PR 👍

Overall the code looks good, but I'd like to try to simplify a bit the code - in particular it took me a while to understand the need for the overriding function.

Thanks for feedback and fast response, hopefully I've simplified it a little now by integrating the precedence logic into projectile-find-matching-test, let me know what you think.

Make changes to projectile-find-implementation-or-test to the effect that it will circumvent the previous call to projectile-find-matching-test if it is detected that :test-dir has been set to a function via projectile--test-file-override.

Adds tests for projectile-find-implementation-or-test, projectile--test-file-override, projectile--complementary-file and projectile--impl-to-test-dir.

State that :test-dir now accepts a function and add a note to the :related-files-fn doc explaining precedence. Also fix up some bad phrasing in related-files-fn doc. Add comparison of the :test-dir and :related-files-fn options.

LaurenceWarne · 2021-06-19T12:15:34Z

projectile.el

-  (if-let ((plist (projectile--related-files-plist-by-kind impl-file :test)))
-      (projectile--related-files-from-plist plist)
-    (if-let ((predicate (projectile--impl-to-test-predicate impl-file)))
+  (if-let ((test-file-from-test-dir-fn


Here is where the :test-dir when set to a function takes precedence over :related-files-fn. If it is not a function, the behaviour should be the same as before.

LaurenceWarne · 2021-06-19T12:17:30Z

@bbatsov Do you have the time to take another look at this? I've added some comments + justifications to the PR so hopefully it should be easier to follow, thanks.

bbatsov · 2021-06-20T05:56:08Z

Looks good! Thanks!

LaurenceWarne commented May 30, 2021

View reviewed changes

LaurenceWarne force-pushed the allow-function-for-test-dir branch from 8e8fe68 to fbd1b2f Compare May 30, 2021 19:22

bbatsov reviewed May 31, 2021

View reviewed changes

LaurenceWarne force-pushed the allow-function-for-test-dir branch from fbd1b2f to b733548 Compare May 31, 2021 17:05

LaurenceWarne commented May 31, 2021

View reviewed changes

LaurenceWarne added 4 commits June 19, 2021 12:58

Implement test-dir as a function logic.

b900669

Make changes to projectile-find-implementation-or-test to the effect that it will circumvent the previous call to projectile-find-matching-test if it is detected that :test-dir has been set to a function via projectile--test-file-override.

Add tests for added and modified functions.

8d5598a

Adds tests for projectile-find-implementation-or-test, projectile--test-file-override, projectile--complementary-file and projectile--impl-to-test-dir.

Add documentation for usage of :test-dir as a function.

21d4535

State that :test-dir now accepts a function and add a note to the :related-files-fn doc explaining precedence. Also fix up some bad phrasing in related-files-fn doc. Add comparison of the :test-dir and :related-files-fn options.

Update changelog.

5b13427

LaurenceWarne force-pushed the allow-function-for-test-dir branch from b733548 to 5b13427 Compare June 19, 2021 12:10

LaurenceWarne commented Jun 19, 2021

View reviewed changes

bbatsov merged commit b8738df into bbatsov:master Jun 20, 2021

LaurenceWarne mentioned this pull request Jun 23, 2021

Allow Function for :src-dir #1679

Merged

6 tasks

LaurenceWarne mentioned this pull request Dec 7, 2021

SBT, maven: projectile doesn't create the test file if the project already contains one with the required name #1650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Functions for :test-dir #1671

Allow Functions for :test-dir #1671

LaurenceWarne commented May 30, 2021 •

edited

Loading

LaurenceWarne May 30, 2021

bbatsov May 31, 2021

bbatsov May 31, 2021

bbatsov May 31, 2021

LaurenceWarne May 31, 2021

bbatsov May 31, 2021

bbatsov May 31, 2021

bbatsov May 31, 2021

bbatsov commented May 31, 2021

LaurenceWarne May 31, 2021 •

edited

Loading

LaurenceWarne commented May 31, 2021

LaurenceWarne Jun 19, 2021

LaurenceWarne commented Jun 19, 2021 •

edited

Loading

bbatsov commented Jun 20, 2021

		@@ -2,11 +2,13 @@

		## master (unreleased)

		* Allow the `:test-dir` option of a project to be set to a function for more flexible test switching.

Allow Functions for :test-dir #1671

Allow Functions for :test-dir #1671

Conversation

LaurenceWarne commented May 30, 2021 • edited Loading

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

bbatsov commented May 31, 2021

LaurenceWarne May 31, 2021 • edited Loading

Choose a reason for hiding this comment

LaurenceWarne commented May 31, 2021

Choose a reason for hiding this comment

LaurenceWarne commented Jun 19, 2021 • edited Loading

bbatsov commented Jun 20, 2021

LaurenceWarne commented May 30, 2021 •

edited

Loading

LaurenceWarne May 31, 2021 •

edited

Loading

LaurenceWarne commented Jun 19, 2021 •

edited

Loading