-
Notifications
You must be signed in to change notification settings - Fork 88
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
Adding the PDU types. #4
Conversation
44fd496
to
a2ec71a
Compare
This pull request is now ready. I've added an
Here is an example output of an echo request.
|
I will be submitting another pull request that will implement an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the efforts deposited on the project so far!
This is the first review iteration. A bunch of suggestions and nitpicks below. I hope that they won't overwhelm you. Please take your time and leave a comment if you're having trouble dealing with any of them.
On a broader perspective, wouldn't it make sense to delegate some of these mechanisms to the encoding crate, which is already dedicated to the reading/writing of DICOM elements and loading text with the intended character set?
Added the PDU types. Added logic for reading/writing PDU types to a stream.
a2ec71a
to
b17d1bc
Compare
76c37b9
to
d7bb21e
Compare
a1ea3b1
to
44ea9dd
Compare
44ea9dd
to
64c3794
Compare
4eea111
to
7c2b802
Compare
I fully expected some changes, so no worries. I made the changes you requested. I'm ready for another review. |
Also, give me a heads up before you merge, and I'll squash these commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the second wave.
Ready for you again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another wave of nitpicks, but it should be ready to merge very soon.
Co-Authored-By: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-Authored-By: Eduardo Pinho <enet4mikeenet@gmail.com>
Ready for you again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ready to become part of the crate roster. Thank you very much!
No description provided.