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

cljr--find-source-ns-of-test-ns can choose the wrong namespace when there are multiple matches #407

Open
orb opened this issue Jan 21, 2018 · 3 comments

Comments

@orb
Copy link

orb commented Jan 21, 2018

The sut can choose the wrong namespace when there are multiple partial matches. For example, given a source tree with:

src/advent/day1.clj
src/advent/day10.clj

Creating a new buffer named test/advent/day10_test.clj results in:

(ns advent.day10-test
  (:require [advent.day1 :as sut]
            [clojure.test :as t]))

When we'd expect:

(ns advent.day10-test
  (:require [advent.day10 :as sut]
                 [clojure.test :as t]))

I have a partial fix at orb@a8306c2. I did not submit a PR because I didn't meet all the contribution requirements yet. However, the basic idea was that instead of finding the first matching file, we'd find the longest matching file.

This solution works well when the sut class exists. If, in this example, src/advent/day10.clj did not exist yet, it would still choose advent.day1 as the best match. I don't like that, but I'm not sure how to best fix that and still maintain the flexibility the code is apparently trying to achieve.

@expez
Copy link
Member

expez commented Jan 21, 2018

Hi, thanks for taking an interest!

I'm not sure if we need two passes (candidates and then final selection). Perhaps the code would just do the right thing if we checked for equality instead?

Given a test buffer /home/expez/moneymaker/test/foo/bar/baz.cljall we'd have to do is replace test with src and we should have an exact match, right?

This solution works well when the sut class exists.

We definitely need to handle the other case too, because some people like to write the tests before the implementation.

@orb
Copy link
Author

orb commented Jan 21, 2018

If the sut class doesn't exist, ideally there would either be no sut import or it should import the "correct" non-existant class, resulting in an immediate error unless we generate the corresponding namespace too.

Take the test namespace, 'day10-test' in this case (with the ns being derived from the buffer name). It looks in the src/advent directory (tranforming test to src) and finds the first file that (after dropping the extension switching - and _) is either a prefix or suffix of day10-test. I assume the logic here is to be able to accommodate both test-* and *-test(s) patterns while failing gracefully (with no sut require) if it can't find a match. However, it current matches much more broadly than that and there may be other patterns we desire to match.

If we only care about the test-* and *-test(s) patterns then I would definitely suggest that instead of my solution we strip any test(s) prefix or suffixes from the test ns and add the sut require only if the corresponding source file exists. I can put together an alternate fix for that for comparison.

@orb
Copy link
Author

orb commented Jan 21, 2018

Looking at the tests, the feature is definitely intended to match aribtrary prefixes and suffixes.

Here's an addition to the test cases the demonstrate the problem:

diff --git a/features/auto-ns.feature b/features/auto-ns.feature
index e0d2078..cbf7954 100644
--- a/features/auto-ns.feature
+++ b/features/auto-ns.feature
@@ -3,6 +3,7 @@ Feature: Add namespace to blank .clj files
   Background:
     Given I have a project "cljr" in "tmp"
     When I have a clojure-file "tmp/src/cljr/core.clj"
+    And I have a clojure-file "tmp/src/cljr/co.clj"

   Scenario: Source file
     When I open file "tmp/src/cljr/core.clj"

This causes multiple failures. Here's one:


  Scenario: Test file
    When I open file "tmp/test/cljr/core_test.clj"
    Then I should see:
      """
      (ns cljr.core-test
        (:require [cljr.core :as sut]
                  [clojure.test :as t]))
      """
      Expected
      (ns cljr.core-test
        (:require [cljr.core :as sut]
                  [clojure.test :as t]))
      to be part of:
      (ns cljr.core-test
        (:require [cljr.co :as sut]
                  [clojure.test :as t]))

@clojure-emacs clojure-emacs deleted a comment from MalloZup Mar 15, 2022
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

No branches or pull requests

2 participants