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

Allow multiple transform streams #165

Closed
wants to merge 1 commit into from
Closed

Conversation

jakepruitt
Copy link

  • breaking change to the API that forces the transform property to be
    an array
  • will require a major version bump and changes downstream

@rclark for the review. Unblocks migration and validation in a tilelive-copy operation.

- breaking change to the API that forces the `transform` property to be
  an array
- will require a major version bump and changes downstream
@rclark
Copy link
Contributor

rclark commented Mar 23, 2016

Thought about this a little more this morning, and as I understand, the primary issue you are running into is the instanceof check here.

We could unblock you in a non-breaking fashion by just dropping this check. It would allow streams built by tools like stream-combiner to be passed as tilelive.copy transforms.

The downside is that a caller who drops in something that can't act like a transform stream will fail with a less focused error message, but I think that's worth the tradeoff of leaving the API unbroken.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 97.685% when pulling 8994920 on multiple-transforms into 0e56c60 on master.

@jakepruitt
Copy link
Author

@rclark okay, closing for now then

@jakepruitt jakepruitt closed this Mar 23, 2016
@jakepruitt jakepruitt deleted the multiple-transforms branch March 23, 2016 21:32
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.

None yet

3 participants