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

refactor run to a es class #694

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jimmywarting
Copy link

No description provided.

Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Tons of changes with no description of why/how these changes improve the codebase and no new tests have been added.

Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

@jimmywarting could you rebase this branch against master? We're trying to breathe some new life into this repo, so sorry for the delay

I think these changes are great by modernizing the code. The changes aren't actually that big as it's mostly moved code. And I don't think new tests are necessary as it's just a refactor. However, this branch should probably be rebased against master to run against the updated test suite.

I can also vouch for @jimmywarting as one of the major authors of the widely used node-fetch library - including by this library. (Though I think it's be nice if we could swap it out for node's new built-in fetch rather than including an extra dependency. @jimmywarting if you've got any extra time, I'd love some help with that (#743))

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