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

Fix cfg syntax errors #1643

Merged
merged 1 commit into from
Apr 2, 2022
Merged

Fix cfg syntax errors #1643

merged 1 commit into from
Apr 2, 2022

Conversation

HebaruSan
Copy link
Contributor

Hi again KSP-RO team,

This is like KSP-RO/RealismOverhaul#2612 but for RP-1.
Referencing KSP-CKAN/CKAN/pull/3525 so I can find this again later easily.

@StonesmileGit
Copy link
Member

Some of these files are generated by a Python script, so will either be handled in that script, or the entries need to be changed in the source data for the script

@HebaruSan
Copy link
Contributor Author

OK, I found the strings in some JSON files and updated them. Does that look better now?

@NathanKell
Copy link
Member

NathanKell commented Mar 29, 2022

So, sadly except for the double @, the extra |, and the missing }, the rest of these are not errors. ConfigNodes explicitly support spaces in keys as well as values (otherwise MM numerical syntax would not work -- @foo *= 5 is actually the kvp @foo * and 5). Due to that, various partmakers put spaces in their partnames. Since the code that uses most of that is comparing a part's name to the key in a confignode, they have to match (or we need to write code to do sanitizing--so this is changeable, but it's not an error, it's just your parser doesn't like it :D ). Note that spaces in nodes are not allowed, of course, which brings us to....leaving the ? in the json, since those are needed to generate @PART[foo?Bar] patches to actually hit parts with names like foo Bar.

@HebaruSan
Copy link
Contributor Author

I was afraid you might say something like that. Allowing spaces would make the syntax so much more messy, but it's so rare (installing RP-1 and all of its recommendations and suggestions and their recommendations and suggestions results in only the examples in this PR plus a few lines from TextureReplacer) that I still had some hope.

I'll split those changes into a separate PR so they don't hold back the ones where we have consensus...

@HebaruSan
Copy link
Contributor Author

OK, this now has just:

  • @@@
  • Remove extra |
  • Add missing }

@NathanKell
Copy link
Member

I was afraid you might say something like that. Allowing spaces would make the syntax so much more messy, but it's so rare (installing RP-1 and all of its recommendations and suggestions and their recommendations and suggestions results in only the examples in this PR plus a few lines from TextureReplacer) that I still had some hope.

I'll split those changes into a separate PR so they don't hold back the ones where we have consensus...

Oh really? Wow. Ok, assuming we can get the part browser's code updated (and the exe rebuilt), the C# side should be simple enough. So that's something we can take on at some point soon. :)

@DRVeyl DRVeyl merged commit 7ca5ec3 into KSP-RO:master Apr 2, 2022
@HebaruSan HebaruSan deleted the fix/cfg-syntax branch April 2, 2022 18:19
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

Successfully merging this pull request may close these issues.

4 participants