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

Possibly fixed #2596 #699

Closed
wants to merge 2 commits into from
Closed

Possibly fixed #2596 #699

wants to merge 2 commits into from

Conversation

Cat-Lady
Copy link

@Cat-Lady Cat-Lady commented Jul 4, 2019

Possible fixed:
ZeroK-RTS/Zero-K-Infrastructure#2596

It seems that some builds of ZK interpret things following '//' (comment mark) as possible markers to resume interpreting, while others not. I have no idea what causes the difference, but I've tested various .json interpreters, and indeed, some of the treat anything past '//' as comment to the end of the line, while others do not.

This commit aims to fix the issue at hand - hoever, in my opinion all our .json files should get scrutinized for comments formating and sanitized, OR reason for the difference in interpreting between ZK builds get discovered and ensured that all know what to do with comment lines.

@Anarchid
Copy link
Member

Anarchid commented Jul 4, 2019

Comments are not part of the JSON format, so if you want to make them valid, you should remove all comments.

CircuitAI as a project has a nonstandard JSON parser that allows comments. Looks like that nonstandard parser is fragile. This is a CircuitAI bug.

If you want to solve the problem and not just the symptom, get CircuitAI to adopt something like hjson instead.

@Cat-Lady
Copy link
Author

Cat-Lady commented Jul 7, 2019

Fixed missing coma that was causing the malformed config errors and "no defenses" bug in AI's below Hard, as per:
ZeroK-RTS/Zero-K-Infrastructure#2596

Circuit moving to -json variant supporting comments natively would still a preferred long-term sanitizing step (editors adhering strictly to .json format and not allowing comments get their language markup frog'ged up and make spotting trivial errors like the missing coma harder).

@GoogleFrog GoogleFrog closed this in bd24674 Jul 8, 2019
@GoogleFrog
Copy link

I don't see why you remove comments from some of the files. You also need to bump configversions.

@Cat-Lady
Copy link
Author

Cat-Lady commented Jul 8, 2019

I'm not sure which of the comments are you referring to:

  1. Some comments were moved (not deleted) to the position that stops confusing .json syntax parsers dujring editing. For example, comment at the end of line 28 makes syntax parsers that adhere to .json standards notice "weight:" in further lines as erroneous (while Shard interprets them all right).

Moving them was a compromise - comments were kept for documentation purposes, but stopped making edits (and spotting errors) harder. I said "compromise", cause non-compromise approach would be to either remove all comments (cause they're not part of .json standard), OR move it to -json variant that incorporates comments.

  1. Some comments were removed, as they were no longer true - like, ones mentioning "fixme: sum of weight should = 1" were removed, cause I remade weights to sum up to full "1".

The comment about bumping versions is, of course true. Hoever - given that the pull request was closed - it isn't relevant anymore, I guess?

@GoogleFrog
Copy link

I am referring to the comments in all the other circuit files.

@Cat-Lady
Copy link
Author

Cat-Lady commented Jul 9, 2019

I just re-checked files changed:
https://github.com/ZeroK-RTS/Chobby/pull/699/files

...and I can't find removed comments. I see only moved comments, covered in point #1 in previous comment.

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.

None yet

3 participants