Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

Conversation

@realityking
Copy link
Contributor

This PR does the following changes

  • Decaffinate the tests
  • Require Node.js 6
  • Add eslint for linting

I'll back out the eslint change into its own PR after a first review as the license issue for eslint-config-airbnb-base still exists.

honzajavorek
honzajavorek previously approved these changes Mar 15, 2018
Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

Wow! Thanks 👍

LGTM. My only concern would be that I'm proponent of the reverse approach:

  1. Decaf the code, leave tests intact, be sure nothing got broken.
  2. Decaf the tests.

However, we have a good past experience with the decaffeination, it's not breaking things, so it probably doesn't matter.

Also, you're right about the airbnb-base license issues.

.travis.yml Outdated

script:
- npm run lint
- npm run test
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: npm test is sufficient

@honzajavorek honzajavorek dismissed their stale review March 15, 2018 11:07

I didn't want to approve technically, but just by words for now. Need to check all licenses etc.

@realityking
Copy link
Contributor Author

realityking commented Mar 17, 2018

I split off linting into #21. That means no dependency changes in this PR🎉

I decaffeinated the actual parser in #22. Feel free to merge that one first, I'll then rebase this one.

Since releasing the decaffeinated code will be semver major (because of the bump to Node.js 6), it'd be great if you could checkout #16 as well.

@realityking realityking mentioned this pull request Apr 24, 2018
@realityking
Copy link
Contributor Author

@michalholasek Rebased!

Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@honzajavorek honzajavorek merged commit e80989e into apiaryio:master May 2, 2018
@realityking realityking deleted the decaffinate-tests branch May 2, 2018 15:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants