Introduce `wrap-ignore-errors` #317

vemv · 2021-07-03T14:24:02Z

Fixes #291

Interestingly, merely by adding intentionally-invalid .clj files to test-resources, all sorts of tests/functionality in the codebase started to break. This surfaced that wrap-ignore-errors is pretty necessary, as otherwise it doesn't seem ideal that the occasional odd file in the source path will render refactor-nrepl useless.

This is particularly true for end users that may have not explicitly set the paths to be analyzed (i.e. they may trying to analyze a whole project, not just a subset like src/test).

The README already advises to set :ignore-errors, so now with this improvement that's more true than ever.

Fixes #291

expez · 2021-07-03T21:29:58Z

I'm strongly opposed to footguns like this (and all our other ignore-errors functionality).

When the user turns on M-x yolo-mode we have no way of distinguishing between actual errors and errors that they want us to DWIM and ignore.

If they have one or more errors in their project that prevents actual analysis and then run cljr-rename-symbol we'll do a half-finished job and their only recourse (because we don't have an undo for this operation) is:

Git reset --hard HEAD
Manually finishing / undoing the job

In a perfect world everyone would be doing red, green, refactor, but that's not how the majority of us works. I for one will very often be working on X and then go off on a tangent to refactor something. If my work on X was incomplete to the point where clj-refactor can't reliable execute my requested refactoring I NEVER want it to just YOLO on. That will turn a 5 second tangent into a huge detour that will drop me out of my state of flow completely.

As far as #291 goes I believe a much better solution would be to let this op too take into account the ignore-paths config setting that let's the user tell us about e.g. their template files.

edit: man, this brings back memories. Benedek and I went so many rounds on ignore-errors back in the day. Happy to have a new audience for my rants 🤣

bbatsov · 2021-07-04T08:06:25Z

Why don’t you use with-safe-transport instead? That’s auto-handled in CIDER and is the way to go in all middle IMO. I planned to push this helper to nREPL itself at some point, but never got to doing so.

…

On Sat, 3 Jul 2021 at 23:30, Lars Andersen ***@***.***> wrote: I'm strongly opposed to footguns like this. When the user turns on M-x yolo-mode they have no way of distinguishing between actual errors and errors that they want us to DWIM and ignore. If they have one or more errors in their project that prevents actual analysis and then run cljr-rename-symbol we'll do a half-finished job and their only recourse (because we don't have an undo for this operation) is: - Git reset --hard HEAD - Manually finishing / undoing the job In a perfect world everyone would be doing red, green, refactor, but that's not how the majority of us works. I for one will very often be working on X and then go off on a tangent to refactor something. If my work on X was incomplete to the point where clj-refactor can't reliable execute my requested refactoring I NEVER want it to just YOLO on. That will turn a 5 second tangent into a huge detour that will drop me out of my state of flow completely. As far as #291 <#291> goes I believe a much better solution would be to let this op too take into account the ignore-paths config setting that let's the user tell us about e.g. their template files. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#317 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZLSTILX4LHZMNE3TCRNLTV56ODANCNFSM47YGA6HA> .

vemv · 2021-07-04T18:04:17Z

Why don’t you use with-safe-transport instead?

Thanks for bringing it up. I don't think it tackles exactly the same problem: with-safe-transport informs of exceptions, while this PR allows functionality to work.

...I'm aware that an end user, after being informed by with-safe-transport of a stacktrace, could fix the corresponding root cause, retry the op and get it to work.

But I think that for a very significant array of use cases, things should work OOTB. I'll write up a refined proposal following @expez' remarks which are certainly useful!

bbatsov · 2021-07-04T20:16:34Z

Fair enough. I’m without a computer for a few days so it’s hard to elaborate my point, but in general I agree with him. And in general I believe that generic functionality should live somewhere higher up the stack, so it’s available to more tools/clients.

On Sun, 4 Jul 2021 at 20:04, vemv ***@***.***> wrote: Why don’t you use with-safe-transport instead? Thanks for bringing it up. I don't think it tackles exactly the same problem: with-safe-transport informs of exceptions, while this PR allows functionality to *work*. ...I'm aware that an end user, after being informed by with-safe-transport of a stacktrace, could fix the corresponding root cause, retry the op and get it to work. But I think that for a very significant array of use cases, things should work OOTB. I'll write up a refined proposal following @expez <https://github.com/expez>' remarks which are certainly useful! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#317 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZLSST52ZVCMIJ7K7MBT3TWCPC3ANCNFSM47YGA6HA> .

-- Best Regards, Bozhidar Batsov

expez · 2021-07-04T22:20:39Z

In the context of cljr-slash in particular I believe the default failure mode should be to fail silently, and just gracefully degrade to simply inserting the / character. However, like @vemv discovered it would also be super useful to be able to toggle explicit errors on (which you can't right now) if you notice the op stops working.

My reasoning behind suppressing errors is that we're hijacking the / button and if we raise an error every time that fails and the user can't, won't or doesn't have time to fix the root cause, it's going to be super annoying real fast.

vemv · 2021-07-05T15:59:02Z

Firstly, it's worth emphasizing the focus of this PR: it ignores errors while building a corpus (namely: reading/requiring files, and sometimes building an AST out of them). It does not propose wrapping the operations themselves (e.g. rename-file-or-dir) in a try/catch.

Building a corpus is somewhat prone to errors whenever the whole project (i.e. not just :source-paths/:test-paths) is being analysed. Which can quite plausibly happen in face of the following considerations:

users may not be even aware of what refactor-nrepl is, especially newcomers or people with different priorities at work
- i.e. it's generally a part of CIDER, so why would a user bother configuring something that is abstracted away by CIDER?
- a good default for this kind of user is working OOTB.
ignore-paths is a blacklist, not a whitelist
- blacklists are always subject to become outdated by new dirs in a given project
- also, importantly, users might work with dozens of projects
  - can we realistically expect that ignore-paths is accurately configured for each of them?
- refactor-nrepl should ideally work OOTB for arbitrary projects, e.g. a random git clone for something I'm checking out transiently
Not everyone sets the refresh-dirs via tools.namespace
- This would simplify some things (since now r-n has some degree of t.n integration)
- but people may not use tools.namespace or leave it with refresh-dirs unconfigured

So, long story short, it's more than plausible that refactor-nrepl can choke on irrelevant files. I don't think that not working (sometimes opaquely) is a good behavior.

Even if refactor-nrepl did accurately inform of the problematic file, asking people to configure their project feels pedantic and may not succeed. One can argue, people want things to 'just work', not a tool give to them more problems :) especially for the mentioned use cases: newcomers, people who work with a lot of projects, or people who prioritize other things over spending time with fine-grained tooling settings.

Anyway, I don't think that the segment of the user base that actually configures things carefully should pay any price for this leniency. So based on @expez input I thought of...

strict-mode

strict-mode would simply a boolean option. Its value would translate directly to the wrap-ignore-errors boolean parameter.

The interesting part being, strict-mode would be set dynamically following the following rules/heuristics:

if strict-mode is explicitly set, honor that; else:
decide its value depending on whether ignore-paths has elements, refresh-dirs is set, etc.
- i.e. any sign that the user knows what he's doing would be taken into account, else we default to leniency.

WDYT?

Between the limited scope of the changes (corpus building) and optionality/smartness of a strict-mode option, this could be a net win in terms of user experience.

vemv · 2021-07-05T16:18:16Z

As one last note, sometimes ASTs can't be built purely becase tools.analyzer errors (bugs). e.g. core.async stuff.

But one can make a sensible (although not risk-free) bet: the code that made t.ana choke was irrelevant for the op in question (e.g. it wouldn't be renamed at all by rename-file-or-dir).

Right now, rename-file-or-dir would fail 100% of the time in face of a problematic file. With the proposed changes, it would fail < 100% of the time.

The result might be slightly invalid, but the user could either fix that manually or discard the results altogether. I don't think that's too much of an ask: no IDE's "rename" is perfect so a seasoned user will have some cautions around automated refactoring (e.g. git-commit prior work, run tests before committing, etc)

expez · 2021-07-05T19:46:00Z

This is probably the one hill I'd be willing to die on when it comes to this project. The setting that allows refactor-nrepl to perform erroneous / incomplete refactorings across your project has to be opt-in.

why would a user bother configuring something that is abstracted away by CIDER?

FWIW, we're not bundled along with CIDER. @benedekfazekas did an amazing job of porting the refactorings that don't require the middleware (e.g. introduce-let and expand-let) to clojure-mode, but the rest has to be explicitly installed.

users may not be even aware of what refactor-nrepl is, especially newcomers or people with different priorities at work

I guess this is the gist of my disagreement about strict-mode and ignore-errors. The target audience of refactor-nrepl is the same as Clojure itself: experts willing to learn the best tool for the job. Making tools for beginners and the disinterested is not where it's at. Instead we want to target the users that are willing to read a short README, learn some new keybindings and maybe even set a variable or two. These are the people that are into Emacs and Clojure and that seek this project out.

can we realistically expect that ignore-paths is accurately configured for each of them?

Many Clojure projects will have at least one Emacs enthusiast and they will add a .dir-locals.el which configures Emacs.

Right now, rename-file-or-dir would fail 100% of the time in face of a problematic file. With the proposed changes, it would fail < 100% of the time.

This line of reasoning is great, because I think we have a lot of ways to sidestep reliability problems related to tools.analyzer if we go on an op-by-op basis. E.g. as you mention rename-file-or-dir it occurs to me that we could probably rely on the same logic that's used by clean-ns to find used symbols instead of tools.analyzer.

The result might be slightly invalid, but the user could either fix that manually or discard the results altogether. I don't think that's too much of an ask: no IDE's "rename" is perfect so a seasoned user will have some cautions around automated refactoring (e.g. git-commit prior work, run tests before committing, etc)

In a previous life I wrote enterprise Java code and I can't recall experiencing Intellij making mistakes on a regular basis. If I ever saw that I would upgrade to the latest version and/or file a bug report. Teaching people to alter their workflow to accommodate the tool is IMO undesirable to begin with but also about as hard as teaching them to configure their tool correctly (which they only have to do once).

expez · 2021-07-05T19:54:56Z

Firstly, it's worth emphasizing the focus of this PR: it ignores errors while building a corpus (namely: reading/requiring files, and sometimes building an AST out of them). It does not propose wrapping the operations themselves (e.g. rename-file-or-dir) in a try/catch.

Yeah, I don't have any objections to this PR. We already have ignore-errors and this looks like an excellent extension of that functionality. I'll give it more thorough read-through and approve (probably tomorrow morning).

I realize I sent us off on a bit of tangent, and I apologize for that, but this line:

The README already advises to set :ignore-errors, so now with this improvement that's more true than ever.

really triggered me 🤣

expez

Great work on this! 👍

expez · 2021-07-06T09:19:48Z

src/refactor_nrepl/ns/tracker.clj

@@ -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?


default-file-predicate or maybe even something like filter-file-predicate? The latter gives some indication that we're using this predicate to filter the files to be processed.

expez · 2021-07-06T09:21:41Z

src/refactor_nrepl/util.clj

+        (f x)
+        (catch Exception e
+          (maybe-log-exception e)
+          ;; return false, because `wrap-ignore-errors` is oriented for predicate usage:


the trailing : in this comment makes it seem like something is missing

the trailing : in this comment makes it seem like something is missing

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 😇

expez · 2021-07-06T09:27:09Z

src/refactor_nrepl/ns/libspecs.clj

-              aliases-by-frequencies)})
+  ([]
+   (namespace-aliases false))
+


Extra line here

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)

expez · 2021-07-06T09:28:21Z

src/refactor_nrepl/ns/tracker.clj

+(defn project-files-in-topo-order
+  ([]
+   (project-files-in-topo-order false))
+


Extra blank line here

expez · 2021-07-06T09:29:22Z

src/refactor_nrepl/rename_file_or_dir.clj

+
+  ([old-path new-path]
+   (rename-file-or-dir old-path new-path false))
+


Extra line here

expez · 2021-07-06T09:38:33Z

src/refactor_nrepl/ns/libspecs.clj

-              sym-by-file&fullname)})
+  ([]
+   (referred-syms-by-file&fullname false))
+


Extra line here

bbatsov · 2021-07-06T10:30:42Z

CHANGELOG.md

@@ -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.


I'd also document this more prominently somewhere, as there are a couple of other editors using refactor-nrepl.

bbatsov · 2021-07-06T10:31:27Z

CHANGELOG.md

+#### 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


This should be a prefix of the parent CL entry.

bbatsov · 2021-07-06T10:31:43Z

CHANGELOG.md

+* 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's a typo in reliability

bbatsov · 2021-07-06T10:33:27Z

test/refactor_nrepl/unreadable_files.clj

@@ -0,0 +1,29 @@
+(ns refactor-nrepl.unreadable-files


You can also add some ns comment about the purpose of the file.

bbatsov · 2021-07-06T10:34:10Z

src/refactor_nrepl/util.clj

@@ -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?]


A docstring is in order for everything part of the public API.

bbatsov · 2021-07-06T10:36:14Z

I've got one concern with the wrap-ignore-errors name - we typically use wrap-something for the names of middleware. Perhaps just ignore-errors or with-suppressed/ignored-errors.

vemv · 2021-07-06T10:46:23Z

FWIW, we're not bundled along with CIDER. [...] has to be explicitly installed.

For completeness, I was referring to this passage of the readme: If you're using CIDER and clj-refactor you don't have to do anything except call cider-jack-in. The dependencies are injected automagically.

The target audience of refactor-nrepl is the same as Clojure itself: experts willing to learn the best tool for the job. Making tools for beginners and the disinterested is not where it's at.

I appreciate this thinking and most definitely wouldn't want to bend the project to accomodate "everyone".

But perhaps by appealing only to an "elite" persona we'd unnecessarily lose many people's trust (by giving a perceived-buggy experience by default, as argued before).

It's not the same to target 40% of devs than 5%. 5% is too little an can end up being counterproductive.

Anyway thanks for the willingness 🤝

vemv

@expez , would you want something like "strict-mode" added to this PR?

Personally I'd actually like it since I wouldn't want gratuitous swallowing either

vemv · 2021-07-06T10:47:56Z

src/refactor_nrepl/ns/libspecs.clj

-              aliases-by-frequencies)})
+  ([]
+   (namespace-aliases false))
+


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)

vemv · 2021-07-06T10:49:25Z

src/refactor_nrepl/util.clj

+        (f x)
+        (catch Exception e
+          (maybe-log-exception e)
+          ;; return false, because `wrap-ignore-errors` is oriented for predicate usage:


the trailing : in this comment makes it seem like something is missing

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 😇

expez · 2021-07-07T10:26:07Z

@expez , would you want something like "strict-mode" added to this PR?

Not really, this PR already contains quite a bit of code and tons of discussion.

I also don't like the idea of strict-mode in general but that discussion is perhaps better moved to a separate issue at this point, if you want to pursue that further.

This probably goes without saying but strict-mode can obviously happen even if I personally don't think it's worth doing. If you're going to be taking on a more active role in the project going forward, @vemv, then it's only fair that your opinion carries more weight. I'd much rather see this project get actively maintained again, and potentially grow some features I disagree with, than seeing it fall into disrepair.

vemv · 2021-07-07T12:34:28Z

I also don't like the idea of strict-mode in general but that discussion is perhaps better moved to a separate issue at this point, if you want to pursue that further.

Yeah always sounds good to seek agreement in advance 👍

This probably goes without saying but strict-mode can obviously happen even if I personally don't think it's worth doing. If you're going to be taking on a more active role in the project going forward, @vemv, then it's only fair that your opinion carries more weight. I'd much rather see this project get actively maintained again, and potentially grow some features I disagree with, than seeing it fall into disrepair.

Ok cheers 🍻 likewise I'd much care about the other maintainer being happy with our creation :)

vemv

PR ready again, will squash the new commit as the last step

vemv · 2021-07-07T13:15:03Z

src/refactor_nrepl/middleware.clj

-(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"))


Not sure if I should compare against true or "true" here?

...Checking out the codebase, I see a handful of "true" handlings. But there's also a boolean coercion in config.clj which makes me hesitate

This is correct, the stuff that's in the msg hasn't yet been coerced to boolean.

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 msg on as is.

bbatsov · 2021-07-08T06:48:24Z

I've got one concern with the wrap-ignore-errors name - we typically use wrap-something for the names of middleware. Perhaps just ignore-errors or with-suppressed/ignored-errors.

@vemv Did you see this remark?

vemv · 2021-07-08T15:02:46Z

Did you see this remark?

Addessed now. Had left it for later as it escaped my eye at first.

Thanks for the feedback both, hope this is, as foreseen, a net gain. Anyway I'll create an issue trying to refine the "strict-mode" idea (might have to be more fine-grained)

Introduce wrap-ignore-errors

a20cbd6

Fixes #291

vemv force-pushed the vemv--291 branch from b83bf54 to a20cbd6 Compare July 3, 2021 14:36

expez approved these changes Jul 6, 2021

View reviewed changes

bbatsov reviewed Jul 6, 2021

View reviewed changes

vemv commented Jul 6, 2021

View reviewed changes

PR feedback

62e4f15

vemv commented Jul 7, 2021

View reviewed changes

wrap-ignore-errors -> with-suppressed-errors

b2c887b

vemv merged commit b78e354 into master Jul 8, 2021

vemv deleted the vemv--291 branch July 8, 2021 15:04


		([old-path new-path]
		(rename-file-or-dir old-path new-path false))

Introduce wrap-ignore-errors #317

Introduce wrap-ignore-errors #317

Uh oh!

Conversation

vemv commented Jul 3, 2021

Uh oh!

expez commented Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbatsov commented Jul 4, 2021 via email

Uh oh!

vemv commented Jul 4, 2021

Uh oh!

bbatsov commented Jul 4, 2021 via email

Uh oh!

expez commented Jul 4, 2021

Uh oh!

vemv commented Jul 5, 2021

strict-mode

Uh oh!

vemv commented Jul 5, 2021

Uh oh!

expez commented Jul 5, 2021

Uh oh!

expez commented Jul 5, 2021

Uh oh!

expez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vemv Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbatsov commented Jul 6, 2021

Uh oh!

vemv commented Jul 6, 2021

Uh oh!

vemv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vemv Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

expez commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vemv commented Jul 7, 2021

Uh oh!

vemv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Introduce `wrap-ignore-errors` #317

Introduce `wrap-ignore-errors` #317

expez commented Jul 3, 2021 •

edited

Loading

vemv Jul 6, 2021 •

edited

Loading

vemv left a comment •

edited

Loading

vemv Jul 6, 2021 •

edited

Loading

expez commented Jul 7, 2021 •

edited

Loading

expez Jul 7, 2021 •

edited

Loading