pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.24k stars 137 forks source link

Rewrite `tree-view.js` #1052

Closed savetheclocktower closed 2 months ago

savetheclocktower commented 2 months ago

Description of the Change

There are some notes in atom-languageclient about features that are blocked on tree-view’s inability to provide “before” callbacks for renaming, creating, and deleting files and directories. I’d like to address this eventually, but the first step is making tree-view easier to maintain.

tree-view.js was a decaffeinated mess. I tried to clean it up a few months back and found out how bad I am at transcribing. Revisited my earlier efforts tonight and found all of my typos. (The commit message says “Decaffeinate tree-view.js” — but it was already decaffeinated, of course. This is just making it more palatable — like adding lots of sugar and cream to a cup of Sanka.)

Alternate Designs

I thought about converting it back to CoffeeScript. (No, that’s a joke.)

Possible Drawbacks

There could be regressions in areas of functionality that aren’t touched by the specs — but I’d be somewhat surprised, since the specs are thorough.

Verification Process

Specs should pass in CI. I’ll also use it for a few days to see if I run into anything.

Release Notes

(Not sure stuff like this warrants release notes entries.)

savetheclocktower commented 2 months ago

I'll leave this in draft for at least a day or so because it's a big change and I want to use it a bit for safety's sake. I might take it out of draft before the 15th, but I understand if folks don't want to get it rubber-stamped for the next release.

savetheclocktower commented 2 months ago

Looks like the one failure is legit. I'll take a look at it tomorrow.

confused-Techie commented 2 months ago

Just adding for posterity, I was the one to originally "decaf" this file. And couldn't agree more it's a total mess.

The issue really stems from the fact that it seems originally tree-view was using CoffeeScript <1.12.7, which is the major cutoff in terms of quality decaffing, since the tooling for that version and lower loses all comments and has the largest offenders in terms of decafed code artifacts.

I had attempted a few times when originally decaffing this to prettify the code, like I did with the other files in that package, but every time I tried it mysteriously seems to have broken something small causing a few tests to fail. (Or maybe I was just doing to much decaf work at the time and messing up something simple lol)

Mauricio even tried themselves later on over on #714 but unfortunately ran into some of the same issues I saw.

But in any case, I'm super excited to see you found success in getting this file to be workable, maybe that's down to the fact that you seem to have also made some changes in the test file (haven't reviewed yet, so that may be incorrect). But whatever it is with tests passing that's already a huge plus to me, especially because I consider the current state of this file to be near unworkable for much of anything, so this is a huge win getting it in a state that it can be modified in the future.

Glad you decided to take a look at it

savetheclocktower commented 2 months ago

But in any case, I'm super excited to see you found success in getting this file to be workable, maybe that's down to the fact that you seem to have also made some changes in the test file (haven't reviewed yet, so that may be incorrect).

Oddly, every change I made to the test file ended up being (I think) extraneous. But I left them in because, once the tests were green, I stopped touching it. :)

In retrospect, what I wish I'd done is hand-converted the entire file from the original CoffeeScript. And that's what I ended up doing in a few places where the generated JS was just too inscrutable for me to understand the code's original intent.