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.

Sign up for GitHub

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

Jump to bottom

Add function for changing charset with unit test. #456

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Add function for changing charset with unit test. #456

merged 2 commits into from
Mar 27, 2024

Conversation

thomas-tribus
Copy link

  • Creates a function to change the charset
  • Adds a unit test to test if the sequences have invalid length when converting into tokens
  • Does not yet change the generation of tokens yet.

Enet4
Enet4 reviewed Feb 14, 2024
View reviewed changes
Copy link
Owner

@Enet4 Enet4 left a 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 initiative, @thomas-tribus! It may be so that we ought to pass charset_changed along into the IntoTokens implementation.

From looking at the test method, I should note that the data set writer might already choose to ignore the encoded element lengths and pick the real byte length instead (#172). This is probably not happening for SQs yet because without the flag, we did not have a way to know in advance whether making the SQ length undefined is needed.

More comments inline.

object/src/mem.rs Show resolved Hide resolved
object/src/mem.rs Show resolved Hide resolved
@Enet4 Enet4 added bug This is a bug C-object Crate: dicom-object labels Feb 14, 2024
@thomas-tribus
Copy link
Author

Thanks @Enet4, I don't have time today, so I'll look at this tomorrow 👍

@thomas-tribus
Copy link
Author

@Enet4 I had a quick look at how I could add the flag to into_token. Could you have a quick look at my latest commit to give an indication if I'm going in the right direction? I don't fully understand how that code fits together. Can you tell me if this approach makes sense?

@thomas-tribus thomas-tribus force-pushed the change/change_charset branch 2 times, most recently from 0becca5 to 07bfd5a Compare February 16, 2024 10:51
@thomas-tribus thomas-tribus force-pushed the change/change_charset branch 2 times, most recently from bfe4c44 to 392d275 Compare February 23, 2024 11:03
thomas-tribus
thomas-tribus commented Feb 23, 2024
View reviewed changes
ul/tests/pdu.rs Outdated Show resolved Hide resolved
@Enet4 Enet4 added the breaking change Hint that this may require a major version bump on release label Feb 23, 2024
@thomas-tribus thomas-tribus marked this pull request as ready for review February 23, 2024 12:05
Enet4
Enet4 reviewed Feb 23, 2024
View reviewed changes
parser/src/dataset/mod.rs Outdated Show resolved Hide resolved
@thomas-tribus thomas-tribus requested a review from Enet4 February 27, 2024 09:44
@Enet4 Enet4 added this to the DICOM-rs 0.7 milestone Mar 13, 2024
@Enet4
Add function for changing charset with unit test.
846a53e 
- Creates a function to change the charset
- Adds a unit test to test if the sequences have invalid length when converting into tokens
- When putting the 'specific charset' token, set a flag to invalidate all lengths when writing the dicom
- In the token generation code, check and pass this token, marking all lengths as UNDEFINED
@Enet4 Enet4 force-pushed the change/change_charset branch from 693c671 to 846a53e Compare March 24, 2024 08:50
Enet4
Enet4 reviewed Mar 24, 2024
View reviewed changes
parser/src/dataset/mod.rs Outdated Show resolved Hide resolved
object/src/tokens.rs Outdated Show resolved Hide resolved
Enet4
Enet4 approved these changes Mar 27, 2024
View reviewed changes
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to bring this in. 👍 Thanks again, @thomas-tribus!

@Enet4 Enet4 merged commit 6e020ec into Enet4:master Mar 27, 2024
4 checks passed
@thomas-tribus
Copy link
Author

thomas-tribus commented Mar 27, 2024 via email

@Enet4 Enet4 added the C-parser Crate: dicom-parser label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Enet4 Enet4 Enet4 approved these changes

Assignees
No one assigned
Labels
breaking change Hint that this may require a major version bump on release bug This is a bug C-object Crate: dicom-object C-parser Crate: dicom-parser
Projects
None yet
Milestone
DICOM-rs 0.7
Development

Successfully merging this pull request may close these issues.
2 participants
@thomas-tribus @Enet4