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

[WIP] KeyValue: Handle (ignore) conditionals #232

Closed
wants to merge 1 commit into from

Conversation

Netshroud
Copy link

As pointed out by @xPaw, we crash when reading tf_english.txt.

I narrowed this down to our handling of conditionals - it's completely broken. We count the conditional as a key, then read the next key as a value, etc. until our keys and values are swapped.

Therefore, an empty value after a conditional will trigger the exception that fails a parse on an empty key.

I don't think this is necessarily the best way to handle them, but at least it doesn't crash 😄

Do not merge (yet), I'm only posting this for discussion. How do we want to handle conditionals, and is this the best way?

I think I can see some implementation flaws in this code (i.e. the green part of the diff) but it's getting late and I need to go to bed so I'm stopping for tonight. I know it's not perfect.

@codecov-io
Copy link

Current coverage is 0.00%

Merging #232 into master will not affect coverage as of 2e504a0

No diff could be generated. No reports for #232 found.
Review entire Coverage Diff as of 2e504a0

Powered by Codecov. Updated on successful CI builds.

@xPaw
Copy link
Member

xPaw commented Feb 3, 2016

Ignoring them is good enough for now, to be honest.

{
return kv.AsString()
.Replace("\r\n", @"\n")
.Replace("\n", @"\n");
Copy link
Member

Choose a reason for hiding this comment

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

Is this a mistake? You're replacing \n with \n.

Copy link
Author

Choose a reason for hiding this comment

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

This replaces <newline> with \n. The @ means an unescaped string :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, that makes sense.

@stil
Copy link
Contributor

stil commented Apr 19, 2016

I made a simple fix for my personal needs. I might add pull request if you think it's useful. It could be sufficient until we have full support for conditionals.

It correctly loads up csgo_english.txt with all 11.7k tokens.

stil/SteamKit@7fcc2418a835c4fba865b8621286211026cd6770

@yaakov-h yaakov-h modified the milestone: 2.next Sep 3, 2017
@yaakov-h yaakov-h changed the title KeyValue: Handle (ignore) conditionals [WIP] KeyValue: Handle (ignore) conditionals Feb 10, 2018
@yaakov-h
Copy link
Member

Closing in favour of new draft PR.

@yaakov-h yaakov-h closed this May 23, 2019
@xPaw xPaw removed this from the 2.next milestone Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants