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/ci-spec #21

Closed
wants to merge 4 commits into from

Conversation

GuilleW
Copy link
Contributor

@GuilleW GuilleW commented Nov 8, 2022

Since I work on many files, I edit package.json to add useful shortcut to avoid misspelt filename.

    "test-decaff": "npx coffee --compile --output spec spec/${npm_config_name}.coffee",
    "test-coffee": "jasmine-focused --captureExceptions --coffee spec/${npm_config_name}.coffee",
    "test-js": "jasmine-focused --captureExceptions spec/${npm_config_name}.js"

Steps to reproduce decaffeination (guidelines):

  • Check package.json for coffee-script version
    • version: 1.12.7
  • Decaffeination with official Coffee program
    • npx coffee --compile --output spec spec/ci-spec.coffee
    • (or my shortcut: ./bin/npm run test-decaff --name=ci-spec)
  • 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

Test from coffee and JS should output same successful result :

From coffee

$ jasmine-focused --captureExceptions --coffee spec/ci-spec.coffee
// or my shortcut $ `./bin/npm run test-coffee --name=ci-spec`
> 4 tests, 8 assertions, 0 failures, 0 skipped

to js

$ jasmine-focused --captureExceptions spec/ci-spec.js
// or my shortcut $ `./bin/npm run test-js --name=ci-spec`
> 4 tests, 8 assertions, 0 failures, 0 skipped

@GuilleW GuilleW marked this pull request as ready for review November 8, 2022 09:51
@GuilleW GuilleW changed the title Decaffeinate ci spec Decaffeinate spec/ci-spec Nov 9, 2022
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.

Overall this PR looks great, just some simple changes requested, otherwise lets get it merged!

})

afterEach(() => {
var done = false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var done = false
let done = false

response.sendFile(path.join(__dirname, 'fixtures', 'native-module-1.0.0.tgz'))
})
server = http.createServer(app)
var live = false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var live = false
let live = false

response.sendFile(path.join(__dirname, 'fixtures', 'native-module-1.0.0.tgz'))
})
server = http.createServer(app)
var live = false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var live = false
let live = false

const apm = require('../lib/apm-cli')

describe('apm ci', () => {
var server
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var server
let server

@2colours
Copy link
Contributor

Supposedly superseeded by #65

@2colours 2colours mentioned this pull request Mar 28, 2023
@confused-Techie
Copy link
Member

Superseded by #65

Thanks again @GuilleW for your fantastic work here, sorry it has taken some time to get it merged in some form. Appreciate your efforts!

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.

3 participants