clean-ns: fall back to :relative-path, when given #251

plexus · 2019-04-18T14:04:11Z

Allow the client to send both the absolute path and a relative path to a file,
we use the first one that we can find.

The absolute path is used when the client (Emacs) and the JVM are on the same
filesystem/same machine. The second is used when they are not on the same
filesystem (e.g. running nREPL on a remote machine or in a VM), but the JVM runs
in the (mounted) project directory, so that relative file lookups still work.

I failed to run the tests locally. Will see what CI says.

See clojure-emacs/clj-refactor.el#380 (comment)

Allow the client to send both the absolute path and a relative path to a file, we use the first one that we can find. The absolute path is used when the client (Emacs) and the JVM are on the same filesystem/same machine. The second is used when they are not on the same filesystem (e.g. running nREPL on a remote machine or in a VM), but the JVM runs in the (mounted) project directory, so that relative file lookups still work. See clojure-emacs/clj-refactor.el#380 (comment)

This makes use of a change to refactor-nrepl [1], but should be safe when using older versions of refactor-nrepl, as in that case it simply uses the absolute path, which is generally preferred over the relative one. When used with the change to refactor-nrepl it will try first the absolute, then the relative path, and use the first that can be found. [1]: clojure-emacs/refactor-nrepl#251

plexus · 2019-04-19T11:10:58Z

FAIL in (sanity) (form-init5821700100748902327.clj:67)
errors handled properly
expected: (string? (:err response))
  actual: (not (string? nil))

This seems unrelated, is this a flaky test?

expez

Looks good to me.

We also need an update to the readme for the clean-ns op and a changelog entry 👍

expez · 2019-04-19T15:15:52Z

src/refactor_nrepl/ns/clean_ns.clj

+(defn clean-ns [{:keys [path relative-path]}]
+  {:pre [(seq path) (string? path)]}
+  ;; Try first the absolute, then the relative path
+  (let [path (first (filter #(some-> % io/file .exists) [path relative-path]))]


some might be better here since we're looking for the first thing that fulfills some pred

Since the predicate returns boolean rather than the truthy value I find it awkward to use some for such cases, as you end up with (some #(and (pred? %) %) coll). Would you still prefer some?

plexus · 2019-04-22T13:40:08Z

I've updated the CHANGELOG and README, and rewrote that use of first + filter.

It's very annoying that I can't run tests locally, no matter what I do I get

clojure.lang.LispReader$ReaderException: java.lang.RuntimeException: No reader function for tag Inf

when mranderson loads the ns parallel.core. I've tried forcing the Clojure version to 1.10 but nothing seems to work.

plexus · 2019-04-22T13:43:40Z

I'm very sorry, I accidentally pushed this to master, and I can't rewind because master is protected. I hope this looks good! I'll try to do some better testing to make sure, let me know if you think this needs more work @expez and I'll do it on a new PR.

expez · 2019-04-23T13:03:40Z

I tried grepping the project for ##Inf but I can't find anything extending the reader to understand that tag. There's also no messing around with *default-data-reader-fn* AFAICT.

@benedekfazekas Does it make a huge difference if you use that library or can we drop it from mranderson? If not, we might consider opening an issue over at that repo to let him know that this is causing breakage over here.

plexus · 2019-04-23T13:10:43Z

Clojure itself understands ##Inf, not 100% sure when it was added, I think maybe 1.9.

It also seems to work on CI so clearly I'm doing something wrong, I just have no clue what. I'm just trying to run make test. I've tried blowing away my ~/.m2 but that didn't help.

expez · 2019-04-23T14:00:11Z

Clojure itself understands ##Inf, not 100% sure when it was added, I think maybe 1.9.

Aha. Did not know that!

It also seems to work on CI so clearly I'm doing something wrong

The version of Clojure that we're using is non-deterministic, since multiple versions are specified. It's just lucky that it works on the CI server. If we added proper exclusions to our project.clj that build profile would no longer run at all, since the dep we pull in through mranderson has a hard dependency on Clojure 1.9.

benedekfazekas · 2019-04-23T14:03:05Z

yeah aware of this new limitation of MrAnderson. however, this only means that the MrAnderson step needs to be run with lein 2.9.1 (older leiningen version may pull an older version of clojure maybe). don't think this should cause any problems downstream, or?

benedekfazekas · 2019-04-23T14:04:56Z

ah checked the above comments properly, guessing @plexus that you use an older version of leiningen?

plexus · 2019-04-23T14:41:53Z

It would appear so, I was still on 2.8.3 apparently.

plexus · 2019-04-23T14:43:36Z

Did a lein upgrade and now it seems to work. Can't believe I didn't check that to begin with 🙈 . Thanks @benedekfazekas !

benedekfazekas · 2019-04-23T15:26:23Z

nw @plexus, happy it is solved

This makes use of a change to refactor-nrepl [1], but should be safe when using older versions of refactor-nrepl, as in that case it simply uses the absolute path, which is generally preferred over the relative one. When used with the change to refactor-nrepl it will try first the absolute, then the relative path, and use the first that can be found. [1]: clojure-emacs/refactor-nrepl#251

jonpither · 2019-07-15T11:34:20Z

When I do a clean-ns using the latest snapshot 2.5.0-SNAPSHOT, on a project with check-out deps, I get:

cljr--get-error-value: Error in nrepl-refactor: java.lang.NullPointerException: null
 at refactor_nrepl.core$cljc_file_QMARK_.invokeStatic (core.clj:129)
    refactor_nrepl.core$cljc_file_QMARK_.invoke (core.clj:127)
    clojure.core$some_fn$sp3__8653.invoke (core.clj:7461)
    refactor_nrepl.core$source_file_QMARK_.invokeStatic (core.clj:152)
    refactor_nrepl.core$source_file_QMARK_.invoke (core.clj:146)
    refactor_nrepl.ns.clean_ns$clean_ns.invokeStatic (clean_ns.clj:44)
    refactor_nrepl.ns.clean_ns$clean_ns.invoke (clean_ns.clj:39)
    clojure.lang.Var.invoke (Var.java:384)
    refactor_nrepl.middleware$clean_ns_reply.invokeStatic (middleware.clj:134)
    refactor_nrepl.middleware$clean_ns_reply.invoke (middleware.clj:133)
    refactor_nrepl.middleware$wrap_refactor$fn__6066.invoke (middleware.clj:212)
    nrepl.middleware$wrap_conj_descriptor$fn__630.invoke (middleware.clj:16)

jonpither · 2019-07-15T11:43:39Z

Updated to the latest clj-refactor.el, now I get:

=> nil
cljr--get-error-value: Error in nrepl-refactor: java.lang.AssertionError: Assert failed: (instance? java.io.PushbackReader rdr)
 at clojure.tools.namespace.parse$read_ns_decl.invokeStatic (parse.clj:33)
    clojure.tools.namespace.parse$read_ns_decl.invoke (parse.clj:33)
    refactor_nrepl.core$read_ns_form.invokeStatic (core.clj:108)
    refactor_nrepl.core$read_ns_form.invoke (core.clj:105)
    refactor_nrepl.core$clj_file_QMARK_.invokeStatic (core.clj:144)
    refactor_nrepl.core$clj_file_QMARK_.invoke (core.clj:139)
    clojure.core$some_fn$sp3__8653.invoke (core.clj:7461)
    refactor_nrepl.core$source_file_QMARK_.invokeStatic (core.clj:152)
    refactor_nrepl.core$source_file_QMARK_.invoke (core.clj:146)
    refactor_nrepl.ns.clean_ns$clean_ns.invokeStatic (clean_ns.clj:44)
    refactor_nrepl.ns.clean_ns$clean_ns.invoke (clean_ns.clj:39)
    clojure.lang.Var.invoke (Var.java:384)
    refactor_nrepl.middleware$clean_ns_reply.invokeStatic (middleware.clj:134)
    refactor_nrepl.middleware$clean_ns_reply.invoke (middleware.clj:133)
    refactor_nrepl.middleware$wrap_refactor$fn__6066.invoke (middleware.clj:212)

benedekfazekas · 2019-07-15T12:10:00Z

that is a know issue with latest snapshot (not really related to this issue) i still have not got around to check. sorry.

plexus mentioned this pull request Apr 18, 2019

clean-ns: send both the absolute and relative path to the server clojure-emacs/clj-refactor.el#438

Closed

Address test failure, fix some reflection warnings.

d56ff99

expez reviewed Apr 19, 2019

View reviewed changes

plexus merged commit d56ff99 into clojure-emacs:master Apr 22, 2019

plexus mentioned this pull request Apr 22, 2019

Use seek instead of find, the latter already exists in clojure.core #252

Merged

plexus deleted the clean-ns-absolute-path-with-fallback-to-relative branch April 23, 2019 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean-ns: fall back to :relative-path, when given #251

clean-ns: fall back to :relative-path, when given #251

plexus commented Apr 18, 2019

plexus commented Apr 19, 2019

expez left a comment

expez Apr 19, 2019

plexus Apr 21, 2019

plexus commented Apr 22, 2019

plexus commented Apr 22, 2019

expez commented Apr 23, 2019

plexus commented Apr 23, 2019

expez commented Apr 23, 2019

benedekfazekas commented Apr 23, 2019

benedekfazekas commented Apr 23, 2019

plexus commented Apr 23, 2019

plexus commented Apr 23, 2019

benedekfazekas commented Apr 23, 2019

jonpither commented Jul 15, 2019

jonpither commented Jul 15, 2019

benedekfazekas commented Jul 15, 2019

clean-ns: fall back to :relative-path, when given #251

clean-ns: fall back to :relative-path, when given #251

Conversation

plexus commented Apr 18, 2019

plexus commented Apr 19, 2019

expez left a comment

Choose a reason for hiding this comment

expez Apr 19, 2019

Choose a reason for hiding this comment

plexus Apr 21, 2019

Choose a reason for hiding this comment

plexus commented Apr 22, 2019

plexus commented Apr 22, 2019

expez commented Apr 23, 2019

plexus commented Apr 23, 2019

expez commented Apr 23, 2019

benedekfazekas commented Apr 23, 2019

benedekfazekas commented Apr 23, 2019

plexus commented Apr 23, 2019

plexus commented Apr 23, 2019

benedekfazekas commented Apr 23, 2019

jonpither commented Jul 15, 2019

jonpither commented Jul 15, 2019

benedekfazekas commented Jul 15, 2019