Opened 12 years ago

Closed 11 years ago

Last modified 10 years ago

#251 closed defect (fixed)

Config parser doesn't ignore comments

Reported by: Philip Taylor Owned by: Quentin Pradet
Priority: Nice to Have Milestone:
Component: Core engine Keywords:
Cc: Patch:


When a config file says something like

key = value ; comment

the config parser thinks the key is associated with two values, "value" and " ; comment". (See mainlog.html, where it reports all the config values that were loaded.)

This needs to be fixed, so legitimate (comma-separated) values are kept but comments are discarded.

Attachments (1)

comments.patch (1.6 KB ) - added by Quentin Pradet 11 years ago.
Patch that remove comments from the config parsing. Tests included.

Download all attachments as: .zip

Change History (12)

comment:1 by Philip Taylor, 12 years ago

Summary: Fix config parserConfig parser doesn't ignore comments
Type: taskdefect

comment:2 by Kevin Read, 12 years ago

Owner: set to Kevin Read
Status: newassigned

I'll look into it.

comment:3 by Quentin Pradet, 11 years ago

binaries/data/config/default.cfg is correctly parsed, and has lines like:

"hotkey.leave = Escape ;End current game or Exit."

I do not dare to use "worksforme", but I'm not sure it is really a bug. How do you reproduce it?

comment:4 by Philip Taylor, 11 years ago

As far as I'm aware, the game still works because it only looks at the first value for each config name (in this case "Escape") and ignores the rest (the comment). Looking in mainlog.html should show the full list of values found by the config parser, which includes the comments.

comment:5 by Quentin Pradet, 11 years ago

Indeed, the parser returns a list of values, and the two first ones are key and value. Since the code that use the parser then take value 0 and 1, the comments are ignored. Isn't that safe since the comments will always end the line?

comment:6 by Philip Taylor, 11 years ago

The config code seems to be using more than just the first key and value, e.g. the log says:

Loaded config std::string "" = "Minus"
Loaded config std::string "" = "NumMinus"
Loaded config std::string "" = " ; Zoom camera out."

so it's picking up the two legitimate values plus the comment, and maybe the comment gets ignored because it's not a valid key name (though I've not looked at the code to see what's going on). It doesn't break the game, but it's printing comments to the log as if they're values, so it's ugly and buggy and would be nice to fix :-)

comment:7 by Quentin Pradet, 11 years ago

Okay. In fact, the parser detects comments, but still returns them as arguments.

Here is a patch fixing that. It assumes that "$rest" will always be a comment. It seems safe to assume that. The only two occurences of $rest in the code are those two line, and are used to parse .cfg files.

parser.InputTaskType("Assignment", "_$ident_= _[-$arg(_minus)]_$value_,>_[-$arg(_minus)]_$value[[;]$rest]"); parser.InputTaskType("CommentOrBlank", "_[;[$rest]]");

Feel free to do whatever you want with the patch, merge it, refuse it, modify it, etc. The licence is BSD.

by Quentin Pradet, 11 years ago

Attachment: comments.patch added

Patch that remove comments from the config parsing. Tests included.

comment:8 by Philip Taylor, 11 years ago

Owner: changed from Kevin Read to Quentin Pradet
Status: assignednew

(Changing assignment since there was no activity from obsidian in a month and I don't want that to block resolution of the issue.)

comment:9 by philip, 11 years ago

Resolution: fixed
Status: newclosed

(In [7109]) Fix #251 (Config parser doesn't ignore comments), based on patch from Cygal

comment:10 by Kevin Read, 11 years ago

Sorry about that, guys, I must admit I got sidetracked and forgot about the issue :(

Nice work cygal!

comment:11 by (none), 10 years ago

Milestone: Open Source Release

Milestone Open Source Release deleted

Note: See TracTickets for help on using tickets.