Opened 9 years ago

Closed 8 years ago

Last modified 7 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:

Description

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 8 years ago.
Patch that remove comments from the config parsing. Tests included.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by Philip Taylor

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

comment:2 Changed 9 years ago by Kevin Read

Owner: set to Kevin Read
Status: newassigned

I'll look into it.

comment:3 Changed 8 years ago by Quentin Pradet

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 Changed 8 years ago by Philip Taylor

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 Changed 8 years ago by Quentin Pradet

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 Changed 8 years ago by Philip Taylor

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

Loaded config std::string "hotkey.camera.zoom.out" = "Minus"
Loaded config std::string "hotkey.camera.zoom.out" = "NumMinus"
Loaded config std::string "hotkey.camera.zoom.out" = " ; 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 Changed 8 years ago by Quentin Pradet

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.

Changed 8 years ago by Quentin Pradet

Attachment: comments.patch added

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

comment:8 Changed 8 years ago by Philip Taylor

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 Changed 8 years ago by philip

Resolution: fixed
Status: newclosed

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

comment:10 Changed 8 years ago by Kevin Read

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

Nice work cygal!

comment:11 Changed 7 years ago by (none)

Milestone: Open Source Release

Milestone Open Source Release deleted

Note: See TracTickets for help on using tickets.