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

Fix NewlineLogOutputDecoder #80

Merged
merged 3 commits into from Jul 4, 2020

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Jul 4, 2020

Currently NewlineLogOutputDecoder works incorrect, decode always produce message even if no \n at end of line. decode also remove newline, so it's not possible distinguish is it was part of message of full message. shiplift implements this correctly, see https://github.com/softprops/shiplift/blob/a4cd2185976ad56b880d5a10374c4dee6d116e6a/src/tty.rs#L79-L147

  • What should be fixed for this change in crate?
  • Since this breaking change, 0.7.0 will be required, is this ok for you?
  • What about return Bytes instead String?

@fussybeaver
Copy link
Owner

fussybeaver commented Jul 4, 2020

Nice.. I was thinking of replacing it with the tokio util LinesCodec ( https://docs.rs/tokio-util/0.3.1/tokio_util/codec/struct.LinesCodec.html ), but this implementation is also good!

Can you adjust the integration tests to add the newline?

@fanatid
Copy link
Contributor Author

fanatid commented Jul 4, 2020

Yeah, I was thought about LinesCodec too, but decide go with own impl because no need looking up \n symbol, we know size of message from header.

What you think about switch to Bytes instead String?

@fussybeaver
Copy link
Owner

Yes, it should be fine to switch to Bytes, it's only used in one endpoint as far as I can see.

@fanatid fanatid force-pushed the fix-log-output-decode branch 4 times, most recently from 1c4979e to b5f2e2c Compare July 4, 2020 17:06
@fanatid
Copy link
Contributor Author

fanatid commented Jul 4, 2020

I changed String to Bytes and fixed integration tests.

@fussybeaver fussybeaver merged commit 555f6a0 into fussybeaver:master Jul 4, 2020
@fussybeaver
Copy link
Owner

Thanks.. I will make a 0.7 release

@fanatid
Copy link
Contributor Author

fanatid commented Jul 4, 2020

Great! Thank you!

@fanatid fanatid deleted the fix-log-output-decode branch July 4, 2020 18:00
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

2 participants