Implementation the StringEncoder and StringDecoder and all them depen… #86
Conversation
|
Hi @avatar29A, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution! TTYL, AZPRBOT; |
|
@avatar29A, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| @@ -15,9 +15,9 @@ namespace DotNetty.Buffers | |||
| /// <summary> | |||
| /// Abstract base class implementation of a <see cref="IByteBuffer"/> | |||
| /// </summary> | |||
| public abstract class AbstractByteBuffer : IByteBuffer | |||
| public abstract class AbstractByteBuffer : IByteBuffer | |||
There was a problem hiding this comment.
please use spaces for indentation
|
CI fell with error "System.Exception: Error during NuGet package creation.". What does it meant? It's normal? |
| int i = index; | ||
| try { | ||
| do { | ||
| if (processor.Process(GetByte(i))) { |
There was a problem hiding this comment.
use _GetByte like in netty to avoid unnecessary index check.
|
@avatar29A, no idea, don't mind it - clearly unrelated to your changes. I've only started the review will continue in ~6 hours. |
|
One more question, if I push new changes in branch, Do you see it, or I need make some advanced actions? |
|
@avatar29A if you push something to the branch you wanted to merge will be seen here. Btw, you probably need to squash commits into one. |
|
@onatm thank's. Yes, I will do it (squash commits into one). |
|
@avatar29A, @onatm actually it's easier for everyone to go through review (might take a series of back-and-forth) and then squash once you get a "LGTM". |
| { | ||
| do | ||
| { | ||
| if (processor.Process(this.GetByte(i))) |
There was a problem hiding this comment.
use _GetByte (like in netty) instead to avoid unnecessary index check.
|
FYI, Still haven't looked through yet, will do in 2h. |
| { | ||
| do | ||
| { | ||
| if (processor.Process(this.GetByte(i))) |
|
|
||
| public override bool Process(byte value) | ||
| { | ||
| if (this.customHandler == null) |
There was a problem hiding this comment.
move check into ctor so that we won't check on every byte during processing
|
@avatar29A now I think I'm through. Code style comments about xml-doc are optional. That's a lot to change and we already have a lot in that format ported over so we can address it later. |
|
NuGet is unhappy about something... not sure what. |
|
Going to re-run CI to see if it's a fluke. Otherwise might be an issue with the version of NuGet.exe being used (might be breaking changes with NuGet) |
|
@Aaronontheweb it's a persistent problem. can you try running without these changes to see if it's ok then? |
|
I'll run it locally and see what the deal is - we had issues with Akka.NET recently too where the latest NuGet.exe blew up our ability to restore and push packages on the fly without modifying the configuration and build script. |
| @@ -666,6 +666,26 @@ public Task WriteBytesAsync(Stream stream, int length, CancellationToken cancell | |||
| return this.buf.WriteBytesAsync(stream, length, cancellationToken); | |||
| } | |||
|
|
|||
| public int ForEachByte(DotNetty.Common.Utilities.ByteProcessor processor) | |||
There was a problem hiding this comment.
move DotNetty.Common.Utilities to usings
|
@avatar29A, left a few more minor comments, LGTM otherwise. Pls, address the comments, squash, and rebase on latest dev. Thanks for taking on this! |
…dencies Modifications - Added LineBasedFrameDecoder.cs - Added MessageToMessageDecoder.cs - Added StringDecoder.cs, StringEncoder.cs - Added ByteProcessor.cs - Added methods ForEachByte in IByteBuffer (also implemented them in AbstractByteBuffer.cs, EmptyByteBuffer.cs, SwappedByteBuffer, WrappedByteBuf); - Added method EncodeString in ByteBufferUtil.cs
|
To my mind, I'm through. Check please. |
Modifications