Fix completion of Java packages/classes on Windows. #79

peterwang · 2021-09-09T01:53:22Z

Since file pathes inside jar files on Windows contain / instead of File/separator, / should also be replaced with . to get
the class name correctly.

Before this fix, completion of java packages such as java.util.*, java.nio.* did not work, the list of candidates was just empty.

cider.nrepl.inlined-deps.compliment.v0v3v11.compliment.sources.namespaces-and-classes> (candidates "java.util." *ns* nil)
()

It completes as expected after this fix.

cider.nrepl.inlined-deps.compliment.v0v3v11.compliment.sources.namespaces-and-classes> (candidates "java.util." *ns* nil)
({:candidate "java.util.concurrent.Callable", :type :class}
 {:candidate "java.util.prefs.AbstractPreferences", :type :class}
 {:candidate "java.util.prefs.BackingStoreException", :type :class}
 {:candidate "java.util.prefs.Base64", :type :class}
 ... 
 {:candidate "java.util.logging.SocketHandler", :type :class}
 {:candidate "java.util.logging.StreamHandler", :type :class}
 {:candidate "java.util.logging.XMLFormatter", :type :class})

Since file pathes inside jar files contain '/' instead of `File/separator', '/' also should be replaced with '.' Before this fix, completion of java packages such as java.util.*, java.nio.* does not work, it works now.

vemv

(contributing a review as someone who helps maintaining cider-nrepl)

I'd add a comment and deftests so that maintainers won't accidentally undo this bugfix in a future.

It also would help to expand the PR description with examples of the problematic strings.

We also should be sure that macOS/Linux users won't be affected (we'd gain that confidence via detailed testing and PR desc)

Cheers - V

peterwang · 2021-09-09T03:11:26Z

Just checked the existing tests, string prefix java.util had been already covered.
I guess that the tests had never been run on Windows previously, so the issue was not caught.
BTW, I just ran lein test on windows: before the fix, there are 13 FAILED tests, after that, there are 6 FAILED tests. It is actually getting better on Windows. Currently, I have no time to further fix the 6 FAILED tests on Windows. Since all tests still run on Linux after the fix, I think is should be safe to merge the fix.

vemv · 2021-09-09T10:43:07Z

Hi, would you consider addressing the simplest things I suggested? Detailed PR desc + inline comment.

Also if you post the output of the failing tests we can try solving them. I don't have a windows machine and setting that up in CI is quite hard (which is why most Clojure projects don't have such a setup sadly).

Failing tests can plausibly mean that other things will be broken on windows so in a way, without a green build everywhere this PR will be of limited use.

Cheers - V

peterwang · 2021-09-09T11:39:55Z

Hi, I have added detailed PR and inline comment for the code change.

Regarding to the FAIED tests on Windows, Please see the following snippets, thanks a lot.

>lein test
lein test
Compiling namespace compliment.context
Compiling namespace compliment.core
Compiling namespace compliment.sources
Compiling namespace compliment.sources.class-members
Compiling namespace compliment.sources.keywords
Compiling namespace compliment.sources.local-bindings
Compiling namespace compliment.sources.namespaces-and-classes
Compiling namespace compliment.sources.ns-mappings
Compiling namespace compliment.sources.resources
Compiling namespace compliment.sources.special-forms
Compiling namespace compliment.utils
WARNING: reader-conditional already refers to: #'clojure.core/reader-conditional in namespace: clojure.tools.reader.impl.utils, being replaced by: #'clojure.tools.reader.impl.utils/reader-conditional
WARNING: tagged-literal already refers to: #'clojure.core/tagged-literal in namespace: clojure.tools.reader.impl.utils, being replaced by: #'clojure.tools.reader.impl.utils/tagged-literal

lein test compliment.sources.t-class-members

lein test compliment.sources.t-keywords

lein test compliment.sources.t-local-bindings

lein test compliment.sources.t-namespaces-and-classes

lein test :only compliment.sources.t-namespaces-and-classes/ns-class-completion

FAIL in (ns-class-completion) (t_namespaces_and_classes.clj:30)
they are completed either according to the mapping in the given
  namespace, or by classpath scanning results
expected: (contains #{"clojure.stacktrace" "clojure.set" "clojure.java.shell" "clojure.string"} :gaps-ok)
  actual: ("clojure.spec.gen.alpha" "cloverage.source" "clojure.stacktrace" "clojure.core.server" "clojure.core.specs.alpha" "clojure.spec.alpha" "clojure.set" "clojure.string")

lein test :only compliment.sources.t-namespaces-and-classes/ns-class-completion

FAIL in (ns-class-completion) (t_namespaces_and_classes.clj:30)
they are completed either according to the mapping in the given
  namespace, or by classpath scanning results
expected: (contains #{{:candidate "clojure.java.shell", :type :namespace} {:candidate "clojure.java.browse", :type :namespace}} :gaps-ok)
  actual: ({:candidate "clojure.java.io", :type :namespace} {:candidate "clojure.java.io.Coercions", :type :class} {:candidate "clojure.java.io.IOFactory", :type :class} {:candidate "clojure.java.api.Clojure", :type :class})

lein test compliment.sources.t-ns-mappings

lein test :only compliment.sources.t-ns-mappings/vars-completion-test

FAIL in (vars-completion-test) (t_ns_mappings.clj:99)
extra metadata can be requested from this completion source
expected: (clojure.core/= [{:candidate "frequencies", :type :function, :ns "clojure.core", :arglists ["[coll]"], :doc "clojure.core/frequencies\n([coll])\n  Returns a map from distinct items in coll to the number of times\n  they appear.\n"}] (binding [*extra-metadata* #{:arglists :doc}] (doall (src/candidates "freq" (-ns) nil))))
  actual: (not (clojure.core/= [{:candidate "frequencies", :type :function, :ns "clojure.core", :arglists ["[coll]"], :doc "clojure.core/frequencies\n([coll])\n  Returns a map from distinct items in coll to the number of times\n  they appear.\n"}] ({:candidate "frequencies", :type :function, :ns "clojure.core", :arglists ("[coll]"), :doc "clojure.core/frequencies\r\n([coll])\r\n  Returns a map from distinct items in coll to the number of times\n  they appear.\r\n"})))

lein test compliment.sources.t-resources

lein test :only compliment.sources.t-resources/resources-test

FAIL in (resources-test) (t_resources.clj:10)
completion works when started from a string in a resource call
expected: (clojure.core/= [{:candidate "META-INF/maven/compliment/compliment/pom.properties", :type :resource}] (src/candidates "META" *ns* (ctx/parse-context (quote (resource "__prefix__")))))
  actual: (not (clojure.core/= [{:candidate "META-INF/maven/compliment/compliment/pom.properties", :type :resource}] ({:candidate "META-INF\\maven\\compliment\\compliment\\pom.properties", :type :resource})))

lein test :only compliment.sources.t-resources/resources-test

FAIL in (resources-test) (t_resources.clj:10)
completion works when started from a string in a resource call
expected: (clojure.core/= [{:candidate "META-INF/maven/compliment/compliment/pom.properties", :type :resource}] (src/candidates "META" *ns* (ctx/parse-context (quote (io/resource "__prefix__")))))
  actual: (not (clojure.core/= [{:candidate "META-INF/maven/compliment/compliment/pom.properties", :type :resource}] ({:candidate "META-INF\\maven\\compliment\\compliment\\pom.properties", :type :resource})))

lein test compliment.sources.t-special-forms

lein test compliment.t-benchmark
Evaluation count : 78 in 6 samples of 13 calls.
             Execution time mean : 8.517454 ms
    Execution time std-deviation : 176.648424 µs
   Execution time lower quantile : 8.309402 ms ( 2.5%)
   Execution time upper quantile : 8.690663 ms (97.5%)
                   Overhead used : 5.784280 ns

lein test compliment.t-context

lein test compliment.t-core

lein test :only compliment.t-core/completions-test

FAIL in (completions-test) (t_core.clj:85)
different metadata is attached to candidates
expected: (just [{:type :namespace, :candidate "clojure.test.tap"}])
  actual: ()

lein test compliment.t-utils

Ran 18 tests containing 196 assertions.
6 failures, 0 errors.
Tests failed.

vemv · 2021-09-09T11:49:05Z

Thanks for the iteration! I'll see what I can make of the failing tests, will let you know.

As for the comment/desc, I'd expect to see what is being exactly fixed / what was exactly failing.

Saying "this fixes x" doesn't quite cut it because it doesn't show why.

src/compliment/utils.clj

vemv · 2021-09-09T12:37:07Z

Inspecting #79 (comment) :

Looks like vars-completion-test can be trivially upgraded from expecting \n to expecting something built off (System/lineSeparator).

resources-test is legitimately failing. The actual value is "META-INF\\maven\\compliment\\compliment\\pom.properties". That doesn't denote a valid resource name; those backslashes should be slashes, even on Windows.

So I'd suggest fixing the other File/separator usage that I can see in the codebase.

@peterwang how does that sound? Would be nice to fix at least a few tests.

peterwang · 2021-09-10T01:05:39Z

Hi, it seems that FAILED tests on Windows were all caused by File/separator. I've fixed the code and the coresponding tests. Regarding the \n problem, I've fixed the test following your suggestion of building the expected string from System/lineSeparator. Finally, all tests run on both Windows and Linux.

vemv

🍻 🚀 great work!

Thanks for the time fixing the tests on Windows.

src/compliment/utils.clj

…iles.

peterwang · 2021-09-10T01:34:15Z

Done with resource-separator, thanks for your guidance.

vemv · 2021-09-30T16:12:38Z

WDYT of these changes? @alexander-yakushev

We're in the process of cutting a new cider-nrepl release, would love to include the current 2 PRs.

alexander-yakushev · 2021-10-21T15:39:41Z

Sorry for leaving you hanging! Merged.

vemv · 2021-10-21T16:23:21Z

Cheers! Looking forward to the release 🍻

Fix completion of Java packages/classes on Windows.

1c9406d

Since file pathes inside jar files contain '/' instead of `File/separator', '/' also should be replaced with '.' Before this fix, completion of java packages such as java.util.*, java.nio.* does not work, it works now.

peterwang mentioned this pull request Sep 9, 2021

Completion of some Java packages/classes does not work on Windows. clojure-emacs/cider-nrepl#708

Closed

vemv reviewed Sep 9, 2021

View reviewed changes

Add inline comment for the change.

47254a8

vemv reviewed Sep 9, 2021

View reviewed changes

src/compliment/utils.clj Outdated Show resolved Hide resolved

Fix issues related to File/separator on Windows.

d286310

vemv approved these changes Sep 10, 2021

View reviewed changes

src/compliment/utils.clj Outdated Show resolved Hide resolved

use resource-separator for path separator within resources and .jar f…

629ecdd

…iles.

alexander-yakushev merged commit 5101d4b into alexander-yakushev:master Oct 21, 2021

alexander-yakushev added the bug label Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix completion of Java packages/classes on Windows. #79

Fix completion of Java packages/classes on Windows. #79

peterwang commented Sep 9, 2021 •

edited

Loading

vemv left a comment •

edited

Loading

peterwang commented Sep 9, 2021 •

edited

Loading

vemv commented Sep 9, 2021

peterwang commented Sep 9, 2021

vemv commented Sep 9, 2021

vemv commented Sep 9, 2021

peterwang commented Sep 10, 2021

vemv left a comment

peterwang commented Sep 10, 2021

vemv commented Sep 30, 2021

alexander-yakushev commented Oct 21, 2021

vemv commented Oct 21, 2021

Fix completion of Java packages/classes on Windows. #79

Fix completion of Java packages/classes on Windows. #79

Conversation

peterwang commented Sep 9, 2021 • edited Loading

vemv left a comment • edited Loading

Choose a reason for hiding this comment

peterwang commented Sep 9, 2021 • edited Loading

vemv commented Sep 9, 2021

peterwang commented Sep 9, 2021

vemv commented Sep 9, 2021

vemv commented Sep 9, 2021

peterwang commented Sep 10, 2021

vemv left a comment

Choose a reason for hiding this comment

peterwang commented Sep 10, 2021

vemv commented Sep 30, 2021

alexander-yakushev commented Oct 21, 2021

vemv commented Oct 21, 2021

peterwang commented Sep 9, 2021 •

edited

Loading

vemv left a comment •

edited

Loading

peterwang commented Sep 9, 2021 •

edited

Loading