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

Binary Model String Encoding #331

Open
AnakinRaW opened this issue Aug 13, 2023 · 1 comment
Open

Binary Model String Encoding #331

AnakinRaW opened this issue Aug 13, 2023 · 1 comment
Assignees
Milestone

Comments

@AnakinRaW
Copy link
Member

AnakinRaW commented Aug 13, 2023

Currently .DAT and .MEG are not alligned regarding how to handle strings in their constructors.
I'm not fully decided for either way but i'd like to bring the discussion up anyway.

Scenario: DataModels like KeyTableRecord (DAT) or MegFileNameTableRecord (Meg) require strings to have Single-byte/ASCII encoding. Their constructors allow to pass in a string value.

Discussion: Do we need data sanitazation for the string value, and if yes, at which place should it get applied?

Option 1 (MEG): The string parameter gets "re-encoded" within the constructor. The model also stores the original string value in a separate property.

Option 2 (DAT): The model is unaware of the encoding when holding the string value. This would require any consumer to ensure correct data. Since the binary models should be internal anyway, this is a valid option.

Btw: has this line key.Replace("\0", string.Empty); a specific purpose? strings in .NET are not zero-terminated, right?

@gruenwaldlk
Copy link
Member

gruenwaldlk commented Aug 13, 2023

In terms of the DAT file I have to enforce a specific encoding otherwise things go south - so I'd probably take care of that inside the library.
The key.Replace("\0", string.Empty); was necessary in the past- It may or may not work without, but I currently don't really have the time (or the machines) to test that for at least Windows 10+ and Linux.

@gruenwaldlk gruenwaldlk added this to the Backlog milestone Nov 15, 2023
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

No branches or pull requests

2 participants