Skip to content
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

Decaffeinate spec/spec-helper (Merge first: it's a dependency) #19

Closed
wants to merge 3 commits into from

Conversation

GuilleW
Copy link
Contributor

@GuilleW GuilleW commented Nov 6, 2022

Helper for unit test.
PR needed before other decaffeination in spec, because this file is included.
I don't remove spec/spec-helper.coffee actually, because it's useful during decaffeination.
⚠️ Should be removed at this end! ⚠️

@sertonix
Copy link
Contributor

sertonix commented Nov 6, 2022

You can remove the .coffee file in a commit but then restore it on your computer without committing it.

@GuilleW
Copy link
Contributor Author

GuilleW commented Nov 6, 2022

decaffeinate-project is used at first, then manual refactor/clean.

@GuilleW
Copy link
Contributor Author

GuilleW commented Nov 7, 2022

Following @confused-Techie comment, I forgot to follow Decaffeination guidelines.

Steps to reproduce decaffeination:

  • Check package.json for coffee-script version
    • version: 1.12.7
  • Decaffeination with official Coffee program
    • npx coffee --compile --output spec spec/spec-helper.coffee
  • Quality of Decaffeination
    • Replace var with const and let if it's possible.
    • Replace Nested Ternary Operators with if statements.
    • Remove return when it's not needed.
    • Remove Result Arrays when they are not needed.
    • Replace for loops with for-of loops.
    • Use Arrow Functions to write functions in a single line when it's possible.
    • Remove unneeded ref variables.
    • Remove do blocks and use let or const instead.
    • Tests and assertions should be the same and success like in coffee file

@GuilleW GuilleW marked this pull request as ready for review November 8, 2022 07:37
@GuilleW GuilleW changed the title decaffeinate spec/spec-helper decaffeinate spec/spec-helper Nov 9, 2022
@GuilleW GuilleW changed the title decaffeinate spec/spec-helper Decaffeinate spec/spec-helper Nov 9, 2022
@GuilleW GuilleW changed the title Decaffeinate spec/spec-helper Decaffeinate spec/spec-helper (Merge first: it's a dependency) Nov 9, 2022
Copy link
Contributor Author

@GuilleW GuilleW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merge 👍

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks fantastic, and I really do appreciate you linking and informing us about how the guide was used @GuilleW.

Thanks a ton for the contribution! 🎉

I'll go ahead and leave this up for a small time longer to ensure any other members are able to take a look otherwise I'll go ahead and merge it

@2colours
Copy link
Contributor

@confused-Techie Well well, it seems that the "small time" has passed. :)

Unfortunately, though, it cannot be merged immediately, since it needs to be consolidated with later changes... My idea would be to collect all changes to these coffee files, since the last commit of this branch, and integrate them into the js code as well. There cannot be that many changes. I can help if you wish.

@confused-Techie
Copy link
Member

@confused-Techie Well well, it seems that the "small time" has passed. :)

Unfortunately, though, it cannot be merged immediately, since it needs to be consolidated with later changes... My idea would be to collect all changes to these coffee files, since the last commit of this branch, and integrate them into the js code as well. There cannot be that many changes. I can help if you wish.

Yeah I'm sorry about that. If it's any consolation, your collection of PRs here helped us determine our proper workflow for decaf PRs, due to the difficulty in accurately reviewing them.

If willing any help would be awesome, but also if fine by you, I'd be willing to restructure these into our new workflow, so that they can finally be reviewed and merged.

Essentially what we have been doing going forward for any decaf work is this:

Firstly create a branch of only machine decaf files.
Then from there have a second branch that does the work of decafing the files manually.
Finally make a PR of your human-decaf branch against the machine-decaf branch.

That way we are able to review what you specifically have done, without caring at all about the actual decaf, since we know the machine decaf will work.

Finally from there, once properly reviewed we can then merge it, but at the last second change the target branch to be the main branch.

But again, sorry for how long these have sat here untouched, if you're willing seeing this as a PR would be amazing, or if it's fine by you, I could do this with your changes, and attribute you as the author on the PRs

@2colours
Copy link
Contributor

This doesn't sound too bad. One can create a new machine-decaf branch from the current state of things, and then merge the updated version of this PR to that. What still needs to be kept in mind is that the main branch still needs to kept frozen, from the generation of the machine-decaf to the eventual merging of it.

I heard you had a discord server, let's talk about it there.

@2colours
Copy link
Contributor

Without further objections, this PR is superseded by #63.

@confused-Techie
Copy link
Member

@2colours Has very helpfully refactored this PR into our modern Decaf process, and has kept your commits in their entirety, so this one has been merged, just on a different PR (#63 )

Thanks a ton to @2colours and @GuilleW for all the hard work here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants