-
Notifications
You must be signed in to change notification settings - Fork 177
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
Add open_file_skipping_BOM function to RC::File #556
Conversation
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.
Please run clang format.
I'm not going to be able to properly review this because I don't know enough about the operations involved in what this PR is trying to accomplish. |
The build script was using utf8-sig, which adds the BOM signature to the start of text files. We need to be able to ignore that when parsing mods.txt |
Yes I know why this is needed, but what I meant was, I'm not qualified to say whether your code is correct and good. |
I'm open to any code that resolves this issue. I imagine there are likely better ways. Theoretically this method is temporarily needed for a release cycle |
but if I understand correctly, all UE4SS files should be utf8, so I really don't know why RC::File is using wstring.. (and that's why I'm trying to make all of them utf8 when porting it..) |
Have we considered taking a 3rd party library dep for this use case? I think it's awesome that we're aiming to stay compatible with the utf8 BOM files we've released, but we also have an opportunity to make our file parsing more robust. Since end users are able to modify config, maybe we should extend to support both utf8 and utf16? |
The third party dep for mods.txt is glz. This is merely a required functionality to maintain backwards compatibility, and this PR shouldn't expand in scope. If someone wants to make a bigger PR with other file parsing functionality they can |
For some reason, its ignoring the fact that all my questions were resolved.
What is the state here? It's quite annoying that the first line is still ignored. |
It's used in #540 |
The other PR is huge, can't we get this change tested and merged first? |
It's used in #540
Yes, and in fact we have to because #540 is dependent on this PR. |
This method won't be used. 540 needs to be rebased to use Yang's method, so this can be closed. |
No description provided.