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

Rewrite from Ruby to Node.js #29

Merged
merged 5 commits into from
Apr 26, 2019
Merged

Conversation

honzajavorek
Copy link
Contributor

@honzajavorek honzajavorek commented Apr 25, 2019

Replacing all Ruby dependencies, Aruba, and 50 lines of Ruby code by 100 lines of Node.js code and a few npm dependencies, which won't collide with any other hooks handler language.

Closes #23, closes #6

I plan to provide Travis CI example and to update the README as a next step - noted in apiaryio/dredd#917 (comment)

@honzajavorek honzajavorek marked this pull request as ready for review April 25, 2019 15:49
@honzajavorek honzajavorek requested a review from kylef April 25, 2019 15:50
package.json Outdated
@@ -2,14 +2,22 @@
"name": "dredd-hooks-template",
"version": "1.0.0",
"description": "A meta-package to install the testing dependencies of the Dredd Hooks template test suite",
"engines": {
"node": ">=10"
Copy link
Member

Choose a reason for hiding this comment

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

Since this is the template for hooks, I would guess that hooks should work on the minimum version of Node that Dredd supports. Is there any reason for limiting to Node >= 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, addressed.



const PROJECT_DIR = path.join(__dirname, '..')
const PROJECT_DIR = path.join(__dirname, '..');
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth adding eslint? (not for this PR though)

const { Given, When, Then, Before, After } = require('cucumber');


Before(function () {
Copy link
Member

Choose a reason for hiding this comment

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

In our ES6 style guide, this and all the closures below are not permitted as they miss function names.

curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py
sudo python get-pip.py
sudo pip install dredd_hooks
command: sudo pip install dredd_hooks
Copy link
Member

Choose a reason for hiding this comment

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

Not saying you have to change this, but you can avoid using sudo with:

$ pip install dredd_hooks --user

Then to use any binary it provides:

$ export PATH="$HOME/.local/bin:$PATH"

I believe Python would automatically pick up the library from ~/.local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's neat. I think it doesn't matter in CI, but it's good to know this is so easy now. (Or was it the same in the past as well? 🤔 )

@honzajavorek
Copy link
Contributor Author

@kylef I plan to add ESlint, but it's one of the last points in apiaryio/dredd#917 (comment). I didn't want to get hold back by that now. Thanks for the review!

@honzajavorek honzajavorek merged commit 262304e into master Apr 26, 2019
@honzajavorek honzajavorek deleted the honzajavorek/remove-ruby branch April 26, 2019 14:25
@ApiaryBot
Copy link
Collaborator

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants