search

jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
260 stars 13 forks source link

Improve mass deletion story / possible allow fixes to remove files #141

Open lydell opened 2 years ago

lydell commented 2 years ago

I recently removed an Elm app from a repo with multiple apps and some shared code.

Basically, I started with removing the app entrypoint. Then I:

  1. Ran elm-review --fix-all.
  2. Accepted all changes.
  3. Searched for NoUnused.Exports: Module in the non-fixable errors output and manually removed every module it told me was unused.
  4. Repeat from the start until no more autofixes.
  5. Fix non-autofixable reports.
  6. Repeat from the start until no errors.

There are two annoyances here:

  1. I got error reports about the last definition in now unused modules, telling me to remove it, and another error telling me to remove the module. If the module is unused and I’m in “mass removal mindset” I don’t care about any errors inside an unused module!
  2. I had to manually remove files and re-run elm-review rather than it chewing along automatically doing most of the work.

We chatted about this on Slack, and @jfmengels brought up a good point of how deleting files can be dangerous:

I find that deleting a file is much scarier than editing a file. If we imagine you’re creating a new file, not using it anywhere, haven’t checked it in Git yet, then you run elm-review --fix-all and accept blindly (which some of us do because we prefer seeing the diff in Git), then you lost your file. No way to get your work back either through Git or your editor (because a lot of them close the file once they detect it’s been removed).

It might be that “mass removal” is like a separate mode of operation. It would be great if that was easier somehow! Because I do remove quite a lot of things somewhat often. Maybe not a whole app, but for example the A version of an A/B test.

Full Slack conversation I have forgotten: Why is it that NoUnused.Exports can tell me that a module is unused, but not remove the whole file? I’m currently in a loop where I run elm-review --fix-all, accept all, search for NoUnused.Exports: Module and delete all files it mentions, repeat. 13 replies jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667807556941489?thread_ts=1667807305.149329&cid=C010RT4D1PT) Because there is no fix for removing a file. (edited) :point_up: 1 Simon Lydell [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667807683741789?thread_ts=1667807305.149329&cid=C010RT4D1PT) I see. Is there an issue on GitHub? jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667807688041919?thread_ts=1667807305.149329&cid=C010RT4D1PT) Also, I find that deleting a file is much scarier than editing a file. If we imagine you’re creating a new file, not using it anywhere, haven’t checked it in Git yet, then you run elm-review --fix-all and accept blindly (which some of us do because we prefer seeing the diff in Git), then you lost your file. No way to get your work back either through Git or your editor (because a lot of them close the file once they detect it’s been removed). jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667807732657479?thread_ts=1667807305.149329&cid=C010RT4D1PT) There isn’t an issue yet, surprisingly. Feel free to create one jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667807805346089?thread_ts=1667807305.149329&cid=C010RT4D1PT) My current thinking, if we allow this, is to separate these fixes on their own. Like even if you use. --fix-all, you’ll get a prompt to only fix that issue (or all similar ones maybe) :+1: 1 jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667807883850119?thread_ts=1667807305.149329&cid=C010RT4D1PT) It’s mostly an issue of convenience / cost of messing up I’d say :+1: 1 jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667807890742239?thread_ts=1667807305.149329&cid=C010RT4D1PT) (Also, you can maybe run it with --watch to tighten the feedback loop) Simon Lydell [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667808369996059?thread_ts=1667807305.149329&cid=C010RT4D1PT) What I’m doing currently is removing one app from a repo containing many Elm apps (with a single elm.json) which share some code. I deleted the app entrypoint and then used elm-review to shake out everything now unused. At this point I just want to delete as much as possible, and then review if there’s on anything that we might have wanted to save (can just restore from git). Then I fix the remaining issues elm-review found. So I understand the concern about deleting things not in Git yet, but in this case (removing old stuff) it’s safe. Just to give some context. jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667808592013879?thread_ts=1667807305.149329&cid=C010RT4D1PT) Yeah. The only concern is if people are editing the file. jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667808673583319?thread_ts=1667807305.149329&cid=C010RT4D1PT) I guess that we could have elm-review check whether the files we are aiming to delete is committed and doesn’t have any changes in the file system. In that case, it’s fine to delete. jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667808899452919?thread_ts=1667807305.149329&cid=C010RT4D1PT) We’d probably need to have the special handling mentioned above (:thinking_face:?). And we’d have a few problems around this: If the file is touched because of other elm-review fixes, then this would not work. Actually it might work for --fix-all, but not for --fix We’d probably use Git to determine whether there are unsaved changes, but some projects might use something else (or don’t use Git), meaning we’d probably need some configuration of the tool (or detection of a .git folder and similar things for other VCS) (edited) jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667809180434209?thread_ts=1667807305.149329&cid=C010RT4D1PT) Ah no it would not work for --fix-all because we’d probably need to single that one out, at which point it’s been touched. jfmengels [3 days ago](https://elmlang.slack.com/archives/C010RT4D1PT/p1667809269392399?thread_ts=1667807305.149329&cid=C010RT4D1PT) The reason why I’m thinking of singling it out is because if we compute the fixes for everything, then notice we’re trying to delete a file that has unsaved changes, then we need to cancel the whole thing (as we don’t know if some fixes have been applied because the file has been considered up for deletion)
jfmengels commented 2 years ago

I'm thinking that adding a flag like --allow-file-delete (in addition to --fix) could make sense. The user would then have to specify whether they're okay with files being deleted.

(Flag name to be reconsidered 😄)

jfmengels commented 2 years ago

I think the same kind of flag could be used to allow editing or creating files as well, which could be very useful when combined with https://github.com/jfmengels/elm-review/issues/138

(and then obviously --allow-file-delete is a wrong name)