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

_debug_agent: use readableObjectMode option for client stream #270

Closed
wants to merge 1 commit into from

Conversation

vkurchatkin
Copy link
Contributor

No description provided.

@bnoordhuis
Copy link
Member

R=@indutny? The change LGTM but maybe there's a reason why it initializes like that.

@indutny
Copy link
Member

indutny commented Jan 10, 2015

LGTM, if it works!

@vkurchatkin
Copy link
Contributor Author

@bnoordhuis
Copy link
Member

@vkurchatkin Can you amend the commit log? The first line should be <= 50 characters.

@piscisaureus
Copy link
Contributor

@vkurchatkin Can you also add a commit message describing the motivation for this change. Because I can't reconstruct why this is needed/nice.

Use public `readableObjectMode` option to construct `Transform`
instead of accessing private `_readableState.objectMode`.
@vkurchatkin vkurchatkin force-pushed the debug-agent-object-mode branch from 8ae8a9c to 566b3e4 Compare January 10, 2015 18:46
@vkurchatkin
Copy link
Contributor Author

@bnoordhuis done
@piscisaureus Just thought that since we have public API to do this it's a good idea to actually use it.

@vkurchatkin
Copy link
Contributor Author

There other places (a lot, actually) where _readableState and _writableState are accessed. Probably something needs to be done about that

@bnoordhuis
Copy link
Member

@vkurchatkin Are you volunteering to clean them up? :-)

@vkurchatkin
Copy link
Contributor Author

I'm afraid it's not as simple as cleaning up. The first thing to do is to categorize all the cases and create issues. In general I see these possible resolutions (sorted by preference):

  • use existing documented API to achieve the same result;
  • introduce new public API;
  • introduce new protected (implementor) API;
  • document some parts of private state for implementors;
  • do nothing.

Can start by doing this)

@chrisdickinson
Copy link
Contributor

Internal streams reaching into / through ._{writ,read}ableState to manipulate fields makes changing the stream api unpleasant, and I'd love to see that cleaned up.

That said, I'm not opposed to going through that process over the course of commits cleaning up smaller transgressions one-by-one. Unless anyone has strong feelings about it, I'd like to merge this PR as-is.

We can track progress on the other places in a separate issue -- @vkurchatkin, would you like to open that up?

@vkurchatkin
Copy link
Contributor Author

@chrisdickinson what about this PR?

@piscisaureus
Copy link
Contributor

lgtm too

@chrisdickinson
Copy link
Contributor

Merging as we speak.

chrisdickinson pushed a commit that referenced this pull request Jan 15, 2015
Use public `readableObjectMode` option to construct `Transform`
instead of accessing private `_readableState.objectMode`.

Partially addresses #445.

PR-URL: #270
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@chrisdickinson
Copy link
Contributor

Merged in 9e62ae4. Thanks! The first shots have been fired in the struggle to cleanup stream usage 💥!

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.

5 participants