-
Notifications
You must be signed in to change notification settings - Fork 69
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
Introduce wrap-ignore-errors
#317
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,18 @@ | |
|
||
## Unreleased | ||
|
||
#### Changes | ||
* The `:ignore-errors` option will be honored in more places, making refactor-nrepl more robust in face of files not particularly meant to be part of the AST corpus. | ||
* Examples: WIP files, Moustache template files, scripts. | ||
* Closes https://github.com/clojure-emacs/refactor-nrepl/issues/291 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a prefix of the parent CL entry. |
||
* Upgrade Orchard | ||
* Worth emphasizing: now the following options are available https://github.com/clojure-emacs/orchard/tree/v0.7.0#configuration-options | ||
* They can make the refactor-nrepl experience simpler / more robust. | ||
* Reliabilty improvement: try using `require` prior to `find-ns` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a typo in reliability |
||
* This increases the chances that a namespace will be found, which in turns makes refactor-nrepl more complete/accurate. | ||
* Replace Cheshire with `clojure.data.json` | ||
* Build ASTs more robustly (by using locks and ruling out certain namespaces like refactor-nrepl itself) | ||
|
||
### Bugs fixed | ||
* [#289](https://github.com/clojure-emacs/refactor-nrepl/issues/289): Fix an edge-case with involving keywords that caused find-symbol to crash. | ||
* [#305](https://github.com/clojure-emacs/refactor-nrepl/issues/305): Don't put `:as` or `:refer` on their own lines in the ns form, when the libspec is so long it causes the line to wrap. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
(disable-warning | ||
{:linter :unused-ret-vals-in-try | ||
:if-inside-macroexpansion-of #{'clojure.test/deftest | ||
'clojure.core/assert}}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,8 +157,8 @@ | |
(delay | ||
(require-and-resolve 'refactor-nrepl.rename-file-or-dir/rename-file-or-dir))) | ||
|
||
(defn- rename-file-or-dir-reply [{:keys [transport old-path new-path] :as msg}] | ||
(reply transport msg :touched (@rename-file-or-dir old-path new-path) | ||
(defn- rename-file-or-dir-reply [{:keys [transport old-path new-path ignore-errors] :as msg}] | ||
(reply transport msg :touched (@rename-file-or-dir old-path new-path (= ignore-errors "true")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I should compare against ...Checking out the codebase, I see a handful of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is correct, the stuff that's in the I suppose we could've done a better job of consistently doing coercions in this ns (like you're doing now) instead of passing the |
||
:status :done)) | ||
|
||
(defn- namespace-aliases-reply [{:keys [transport] :as msg}] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
(ns refactor-nrepl.ns.libspecs | ||
(:require [refactor-nrepl.core :as core] | ||
[refactor-nrepl.ns.ns-parser :as ns-parser]) | ||
[refactor-nrepl.ns.ns-parser :as ns-parser] | ||
[refactor-nrepl.util :as util]) | ||
(:import [java.io File])) | ||
|
||
;; The structure here is {path {lang [timestamp value]}} | ||
|
@@ -46,13 +47,18 @@ | |
|
||
{:clj {util com.acme.util str clojure.string | ||
:cljs {gstr goog.str}}}" | ||
[] | ||
{:clj (->> (core/find-in-project (some-fn core/clj-file? core/cljc-file?)) | ||
(map (partial get-libspec-from-file-with-caching :clj)) | ||
aliases-by-frequencies) | ||
:cljs (->> (core/find-in-project (some-fn core/cljs-file? core/cljc-file?)) | ||
(map (partial get-libspec-from-file-with-caching :cljs)) | ||
aliases-by-frequencies)}) | ||
([] | ||
(namespace-aliases false)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra line here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will rm it (fwiw it was there on purpose... I don't like "shim" arities to clutter the "impl" arity, so that I can read the impl better) |
||
([ignore-errors?] | ||
{:clj (->> (core/find-in-project (util/wrap-ignore-errors (some-fn core/clj-file? core/cljc-file?) | ||
ignore-errors?)) | ||
(map (partial get-libspec-from-file-with-caching :clj)) | ||
aliases-by-frequencies) | ||
:cljs (->> (core/find-in-project (util/wrap-ignore-errors (some-fn core/cljs-file? core/cljc-file?) | ||
ignore-errors?)) | ||
(map (partial get-libspec-from-file-with-caching :cljs)) | ||
aliases-by-frequencies)})) | ||
|
||
(defn- unwrap-refer | ||
[file {:keys [ns refer]}] | ||
|
@@ -75,10 +81,15 @@ | |
Example: | ||
{:clj {\"/home/someuser/projects/some.clj\" [\"example.com/foobar\" foobar]} | ||
:cljs}" | ||
[] | ||
{:clj (->> (core/find-in-project (some-fn core/clj-file? core/cljc-file?)) | ||
(map (juxt identity (partial get-libspec-from-file-with-caching :clj))) | ||
sym-by-file&fullname) | ||
:cljs (->> (core/find-in-project (some-fn core/cljs-file? core/cljc-file?)) | ||
(map (juxt identity (partial get-libspec-from-file-with-caching :cljs))) | ||
sym-by-file&fullname)}) | ||
([] | ||
(referred-syms-by-file&fullname false)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra line here |
||
([ignore-errors?] | ||
{:clj (->> (core/find-in-project (util/wrap-ignore-errors (some-fn core/clj-file? core/cljc-file?) | ||
ignore-errors?)) | ||
(map (juxt identity (partial get-libspec-from-file-with-caching :clj))) | ||
sym-by-file&fullname) | ||
:cljs (->> (core/find-in-project (util/wrap-ignore-errors (some-fn core/cljs-file? core/cljc-file?) | ||
ignore-errors?)) | ||
(map (juxt identity (partial get-libspec-from-file-with-caching :cljs))) | ||
sym-by-file&fullname)})) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
[repl :refer [refresh-dirs]] | ||
[track :as tracker]] | ||
[refactor-nrepl.core :as core] | ||
[refactor-nrepl.util :as util] | ||
[refactor-nrepl.ns.ns-parser :as ns-parser]) | ||
(:import [java.io File])) | ||
|
||
|
@@ -36,13 +37,16 @@ | |
;; corner case - use the mranderson-ized refresh-dirs (for supporting this project's test suite): | ||
refresh-dirs)) | ||
|
||
(def default-predicate (every-pred core/source-file? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
safe-for-clojure-tools-namespace?)) | ||
|
||
(defn build-tracker | ||
"Build a tracker for the project. | ||
|
||
If file-predicate is provided, use that instead of `core/source-file?`" | ||
([] | ||
(build-tracker #(and (core/source-file? %) | ||
(safe-for-clojure-tools-namespace? %)))) | ||
(build-tracker default-predicate)) | ||
|
||
([file-predicate] | ||
(file/add-files (tracker/tracker) (core/find-in-project file-predicate)))) | ||
|
||
|
@@ -74,11 +78,16 @@ | |
(boolean (some #(str/starts-with? % file-as-absolute-paths) | ||
refresh-dirs-as-absolute-paths))))) | ||
|
||
(defn project-files-in-topo-order [] | ||
(let [tracker (build-tracker (every-pred (partial in-refresh-dirs? (user-refresh-dirs)) | ||
core/clj-file?)) | ||
nses (dep/topo-sort (:clojure.tools.namespace.track/deps tracker)) | ||
filemap (:clojure.tools.namespace.file/filemap tracker) | ||
ns2file (zipmap (vals filemap) (keys filemap))] | ||
(->> (map ns2file nses) | ||
(remove nil?)))) | ||
(defn project-files-in-topo-order | ||
([] | ||
(project-files-in-topo-order false)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra blank line here |
||
([ignore-errors?] | ||
(let [tracker (build-tracker (util/wrap-ignore-errors (every-pred (partial in-refresh-dirs? (user-refresh-dirs)) | ||
core/clj-file?) | ||
ignore-errors?)) | ||
nses (dep/topo-sort (:clojure.tools.namespace.track/deps tracker)) | ||
filemap (:clojure.tools.namespace.file/filemap tracker) | ||
ns2file (zipmap (vals filemap) (keys filemap))] | ||
(->> (map ns2file nses) | ||
(remove nil?))))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -194,10 +194,11 @@ | |
|
||
(defn- rename-source-file | ||
"Move file from old to new, updating any dependents." | ||
[old-path new-path] | ||
[old-path new-path ignore-errors?] | ||
(let [old-ns (core/ns-from-string (slurp old-path)) | ||
new-ns (path->ns new-path) | ||
tracker (tracker/build-tracker) | ||
tracker (tracker/build-tracker (util/wrap-ignore-errors tracker/default-predicate | ||
ignore-errors?)) | ||
dependents (tracker/get-dependents tracker old-ns) | ||
new-dependents (atom {})] | ||
(doseq [^File f dependents] | ||
|
@@ -214,15 +215,17 @@ | |
[path old-parent new-parent] | ||
(str/replace path old-parent new-parent)) | ||
|
||
(defn- rename-dir [old-path new-path] | ||
(defn- rename-dir [old-path new-path ignore-errors?] | ||
(let [old-path (util/normalize-to-unix-path old-path) | ||
new-path (util/normalize-to-unix-path new-path) | ||
old-path (if (.endsWith old-path "/") old-path (str old-path "/")) | ||
new-path (if (.endsWith new-path "/") new-path (str new-path "/"))] | ||
(flatten (for [^File f (file-seq (File. old-path)) | ||
:when (not (fs/directory? f)) | ||
:let [path (util/normalize-to-unix-path (.getAbsolutePath f))]] | ||
(-rename-file-or-dir path (merge-paths path old-path new-path)))))) | ||
(-rename-file-or-dir path | ||
(merge-paths path old-path new-path) | ||
ignore-errors?))))) | ||
|
||
(defn- file-or-symlink-exists? [^String path] | ||
(let [f (File. path)] | ||
|
@@ -234,12 +237,12 @@ | |
(when (.. target toFile exists) | ||
path))))))) | ||
|
||
(defn- -rename-file-or-dir [^String old-path new-path] | ||
(defn- -rename-file-or-dir [^String old-path new-path ignore-errors?] | ||
(let [affected-files (if (fs/directory? old-path) | ||
(rename-dir old-path new-path) | ||
(rename-dir old-path new-path ignore-errors?) | ||
(if ((some-fn core/clj-file? core/cljs-file?) | ||
(File. old-path)) | ||
(rename-source-file old-path new-path) | ||
(rename-source-file old-path new-path ignore-errors?) | ||
(rename-file! old-path new-path)))] | ||
(->> affected-files | ||
flatten | ||
|
@@ -271,7 +274,11 @@ | |
old-path and new-path are expected to be aboslute paths. | ||
|
||
Returns a list of all files that were affected." | ||
[old-path new-path] | ||
(assert-friendly old-path new-path) | ||
(binding [*print-length* nil] | ||
(-rename-file-or-dir old-path new-path))) | ||
|
||
([old-path new-path] | ||
(rename-file-or-dir old-path new-path false)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra line here |
||
([old-path new-path ignore-errors?] | ||
(assert-friendly old-path new-path) | ||
(binding [*print-length* nil] | ||
(-rename-file-or-dir old-path new-path ignore-errors?)))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,3 +72,14 @@ | |
(defn maybe-log-exception [^Throwable e] | ||
(when (System/getProperty "refactor-nrepl.internal.log-exceptions") | ||
(.printStackTrace e))) | ||
|
||
(defn wrap-ignore-errors [f ignore-errors?] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A docstring is in order for everything part of the public API. |
||
(if-not ignore-errors? | ||
f | ||
(fn [x] | ||
(try | ||
(f x) | ||
(catch Exception e | ||
(maybe-log-exception e) | ||
;; return false, because `wrap-ignore-errors` is oriented for predicate usage: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the trailing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I add the colon to make it clear that the comment refers to the next LOC. Else there can be an ambiguity (does it refer to the previous one, to the defn in general, etc?) I guess it failed at that 😇 |
||
false))))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
;; An incorrect alias usage (because `a` is not declared in the `ns` form): | ||
::a/foo |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
;; An incorrect data readers usage (because there's no such data reader declared anywhere): | ||
#totally.made.up/foo [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
(ns unreadable-file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also document this more prominently somewhere, as there are a couple of other editors using
refactor-nrepl
.