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

first run at 6.4.0 #225

Merged
merged 4 commits into from
Sep 6, 2016
Merged

first run at 6.4.0 #225

merged 4 commits into from
Sep 6, 2016

Conversation

calvinmetcalf
Copy link
Contributor

and there is a new es6 feature being used in tests and a couple more fixtures!

@calvinmetcalf
Copy link
Contributor Author

and a few tests that use encodings not all versions of node have, no actual changes to streams here so no rush

@Fishrock123 Fishrock123 changed the title fist run at 6.4.0 first run at 6.4.0 Aug 22, 2016
@calvinmetcalf
Copy link
Contributor Author

ok the issue with the preprocessing test was basically it was relying on some specific tty stuff that was kinda tangential to streams specifically

@calvinmetcalf
Copy link
Contributor Author

if others are on the same page about being able to ditch that preprocess test I can merge this

@mcollina
Copy link
Member

@calvinmetcalf LGTM

As a side note, that test is not targeting readable-stream, as it is using fs, readline and others`, so there is nothing we can fix here.

Can we avoid pulling it the next time? That test also relies on two txt files, those should be removed as well. It's really a test about fs and readline rather than streams.

As a side note, that test it was introduced by @vsemozhetbyt in nodejs/node#7741 and nodejs/node@ea725ed

cc @addaleax

@calvinmetcalf
Copy link
Contributor Author

oh right need to remove those

@calvinmetcalf
Copy link
Contributor Author

we'd avoid pulling it in if it wasn't named 'stream' it's just a function of the fact that it was touching 3 diff modules and it had to pick one

@mcollina
Copy link
Member

@calvinmetcalf we should move it then.

@addaleax
Copy link
Member

I’m not sure what the process for pulling in tests is but if moving that one test in the node repo makes anything easier for anyone that shouldn’t be a problem?

@calvinmetcalf
Copy link
Contributor Author

not a big deal any more, I just put something in the code that doesn't pull
it in

On Wed, Aug 31, 2016 at 3:11 PM Anna Henningsen notifications@github.com
wrote:

I’m not sure what the process for pulling in tests is but if moving that
one test in the node repo makes anything easier for anyone that shouldn’t
be a problem?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#225 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABE4n1fsktxGhufYo5JvHPu_77t_-Clfks5qldHugaJpZM4Jm1kx
.

@calvinmetcalf
Copy link
Contributor Author

this is a transient error, I'll plan on merging this latter if nobody objects

@calvinmetcalf calvinmetcalf merged commit 3e8da33 into master Sep 6, 2016
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