Suggestion about decaffeination

[GuilleW]

Problem: review is hard/long after someone decaffeinate.
After reading this PR (pulsar-edit/ppm#15 (comment)) and talk on Discord (somewhere, sorry too lazy to search for it 😝 ), here's my suggestion to go forward and make review faster and easier (and I know you want it 😉 ). It follow guidelines.

Waiting your approval @DeeDeeG, @confused-Techie, folks ?

Pull Request 1 : Decaffeination (with no manual change)

• Decaffeination with official (and bundled) Coffee program:

  npx coffee --compile --output lib src 
  find ./src -name '*.coffee' -exec rm {} \;
  
  npx coffee --compile spec
  find ./spec -name '*.coffee' -exec rm {} \;

• Create a Hash of decaffeinate files

  find ./lib -type f -print0 | sort -z | xargs -0 sha1sum | sha1sum
  find ./spec -type f -print0 | sort -z | xargs -0 sha1sum | sha1sum

• Pull Request
  Just PR all decaffeinate files of a folder (like src or spec). and add you hash in comment.

• Reviewer
  You just need to run same command and check if you get same hash. So you don't waste your time checking each files.
  If you trust the command, you trust the result (and PR). Review will just takes few seconds!

• Merge

Pull Request 2 : Remove the wrapping code in function

I'll write a bash/NodeJS or [whatever you're ok] script to remove this:

// Generated by CoffeeScript 1.12.7
(function() {
  {...}
}).call(this);

and all extra space (indent) in the wrapper.

• Pull Request

• Reviewer
  You just need to check if 2 first lines were removed and two last lines (+ 2 space indent in the wrapper). Review will just takes few seconds!

• Merge

Pull Request 3 : Refactor following guidelines

At this point, refactor should not change lots of lines, we can PR files by 5, 10 or all ? Always keep in mind the time to review should not take more than 1-2 minutes.

Todo

• ppm/src
  • Decaffeination
  • Remove wrapper
  • Refactor
• ppm/spec
  • Decaffeination
  • Remove wrapper
  • Refactor

[sertonix]

The decaffeinate commands should be npx coffee --compile --output lib src and npx coffee --compile spec.
The hash is a very good idea. But it is missing explanation wich path is hashed. Maybe mention the lib and spec directory. Also it should be ensured that when decaffeinating into the spec directory itself all old files are deleted before generating the hash.
Alternatively the spec directory could be decaffeinated to spec-out, then the spec directory is deleted and spec-out renamed to spec.

But since this is not an instruction for users it doesn't need to include all these informations.

Also maybe rename Step to pull request to make clear that they are separate pull requests

[GuilleW]

Update from your suggestions, thanks!

[DeeDeeG]

Hi @GuilleW, thanks for taking the initiative here.

Regarding "Pull Request 1 : Decaffeination (with no manual change)"

We don't have to manually calculate hashes, Git hashes pretty much every file it knows about. You can checkout the decaf branch and re-run the coffee compiler yourself, and if there is any diff (git status will show if there are changes, git diff will show the difference line by line) then the machine compilation is not committed in the decaf and/or somebody made a mistake. Hopefully that doesn't happen.

If people don't mind, I'd like to do this step myself, uploading the result to its own branch so as not to block the master branch. At which point, no Pull Request is needed for this step. I want to just get us to have a good foundation to start from that "just works" before opening it up for collaboration.

Although if people are impatient to get CoffeeScript gone right away on master branch, doing any new work onlly in JavaScript, then I will do this as a PR against master branch... But my default position is to do it on a separate "decaf" branch and manually sync in any CoffeeScript changes that happen to land at master branch myself, which keeps the branches in sync without blocking progress on master.

See the decaf-central branch I just posted: pulsar-edit/ppm@master...decaf-central

This is my suggested base for the rest of manual decaffeination changes.

Regarding "Pull Request 2 : Remove the wrapping code in function"

As long as this approach passes automated CI testing, then it should be okay. I think it may be important for the main apm-cli.js to be self-executing like that, since it needs to do something when you run it, or else the CLI is broken! But it will be much easier to see what is going when PR 1 is done or when starting from my decaf-central branch, and now that CI on this repo is generally passing. So it's worth trying this and seeing what we learn from it.

Regarding "Pull Request 3 : Refactor following guidelines"

I am a little nervous this will devolve into minor code style disagreements, where the code does effectively the same thing but we dispute how best to re-write it. So I hope we can avoid intractable bikeshedding. But... in theory yes, this is the main point, so if people still feel up for trying this, then let's try it. Patience please, everyone, and let's make sure we are realistic about time/effort required when starting something or when asking for changes during review.

cc @pulsar-edit/core for your thoughts.

[confused-Techie]

I do agree that starting off with a machine translated source is best. From there, aiming PR's at the branch that's decaffed sounds great to me. And it's at that point we can go through and fix any errors or issues that may arise. But otherwise personally I'd say just to keep it all one PR when making adjustments. Not everything has to be in the same PR, but with the small team of reviewers we do have that feels most realistic

[DeeDeeG]

Regarding one PR at a time: 👍 Yeah, that sounds about right. I personally think that whatever work we have posted for review should be in one PR, however big or small it is. (And other work not posted for final review until the first open PR is merged or closed or whatever.)

One exception: I would understand src/ decaf and spec/ decaf happening in separate PRs. It would be great if the src/ decaffeination PR(s) passed CI without touching the tests (spec/ folder) at all, that would be super. EDIT: I would really prefer if spec/ decaf was not reviewed until src/ decaf was pretty much done, if possible? So we know the old tests are working just like they were written and the src/ decaf was done correctly, and then we very carefully decaffeinate the specs only afterward?

(With the benefit of hindsight I can say: Multiple related small PRs open at once was a fragmented experience that wasn't very efficient for anyone. I know it was not intentional for it to be hard to review like that, but that is what ended up happening, and I personally didn't see that coming as a problem until it was already happening.)

[confused-Techie]

@DeeDeeG totally agree that the tests themselves shouldn't be touched until we know everything else works. Otherwise we could have bugs pilling on bugs without us noticing. So good suggestion there

[confused-Techie]

With these suggestions, I've tried to implement the form we are talking about here over on first-mate, that is a branch was made with the machine decaf code available. With 0 changes. Then a PR to manually decaf some files was made there. I did it with only a few of the files, so we could get a feel if this works, before commiting to the whole repo. Beyond that, the tests have been modified slightly, to require from the JS files and run their tests, which at least locally are passing just fine.

If this looks good to everyone, I could continue to do the whole repo, and see what we think. But at least to me, this seems a lot more manageable with the fantastic suggestions from everyone in this thread.

[sertonix]

While reading confused-Techie's pull request I remembered another disadvantage of the normal coffeescript transpiler:
Single line comments are not converted. So they have to be re-added manually.