alliedmodders / amxmodx

AMX Mod X - Half-Life 1 Scripting and Administration
http://www.amxmodx.org/
489 stars 197 forks source link

Make ini parser format same as other normal formats. #800

Open afwn90cj93201nixr2e1re opened 4 years ago

afwn90cj93201nixr2e1re commented 4 years ago

Description

Now seems like it's using space as strtok value

Problematic Code (or Steps to Reproduce)

say /command = value
gonna be translated to ->
key = say | value = value
Which is incorrect for INI files
Arkshine commented 4 years ago

It's by design. Identifier accepts only A-Z a-z 0-9 _ - , + . $ ? / That's said, it's not bad idea.

Did you try to put the key between "? Maybe it will work.

Also, you have the possibility to use the SMC format.

afwn90cj93201nixr2e1re commented 4 years ago

So then it's gonna return "key" instead key, you know, but ini always parsing first token then = and then second one, it's ignore spaces between =, ini parser now is very bad designed, see previous issue for more info too, seems like it's totally depended on SMC parser, which should have another one logic.

I used this one in ExtraMirror:

изображение

Arkshine commented 4 years ago

You can always use remove_quotes for the key if you want to use non-standard format. This does not seems a blocking issue.

Same for value, you can use " if you need spaces at the beginning. Though for this one, it's by design, and you don't need remove_quotes.

afwn90cj93201nixr2e1re commented 4 years ago

So, i know that i can get avoid without amxmodx changes with removing quotes and other stuff like that, but then that's not true ini file. INI parser should be fully rewrited ~and loose SMC parser dependens~. изображение

afwn90cj93201nixr2e1re commented 4 years ago

I just wanna say that i just rewrite logic just for fixing that bug, that not nice Before: изображение

After: изображение

As you can see the first one was light, but now i added too much checks to code for formating strings as they should work just for ini parser bad design.

Arkshine commented 4 years ago

Both INI and SMC format are parsed differently. I'm not sure to understand what you're talking about. Also, the original INI format doesn't allow spaces in the key.

If you want to use spaces in keys, surround them with " and use remove_quotes ; it should work.

afwn90cj93201nixr2e1re commented 4 years ago

https://github.com/alliedmodders/amxmodx/blob/master/amxmodx/CTextParsers.cpp#L996

afwn90cj93201nixr2e1re commented 4 years ago

Also, the original INI format doesn't allow spaces in the key.

It's also doesnt allow to parse key values without '=' token.

afwn90cj93201nixr2e1re commented 4 years ago

https://en.wikipedia.org/wiki/INI_file#Format

Arkshine commented 4 years ago

Implementation can vary, but that's not the point. What is matter is: does the solution provided above work for you?

afwn90cj93201nixr2e1re commented 4 years ago

Yes it is, i already known about this solution when started PR.

Arkshine commented 4 years ago

Great. That's not what you said here though: https://github.com/alliedmodders/amxmodx/issues/800#issuecomment-557807348 There is no bug as you affirmed it. You can use " for the key.

I don't know if the suggestion actually makes sense, but it's noted.

afwn90cj93201nixr2e1re commented 4 years ago

Yes, bug -> unexpected rule, which is not covered by original specification.

Arkshine commented 4 years ago

The current spec for AMXX is available in the include:

/**
 * The INI file format is defined as:
 *    WHITESPACE: 0x20, \n, \t, \r
 *    IDENTIFIER: A-Z a-z 0-9 _ - , + . $ ? / 
 *    STRING    : Any set of symbols
 * 
 * Basic syntax is comprised of SECTIONs.
 *    A SECTION is defined as:
 *    [SECTIONNAME]
 *    OPTION
 *    OPTION
 *    OPTION...
 *
 * SECTIONNAME is an IDENTIFIER.
 *    OPTION can be repeated any number of times, once per line.
 *    OPTION is defined as one of:
 *      KEY = "VALUE"
 *      KEY = VALUE
 *      KEY
 *    Where KEY is an IDENTIFIER and VALUE is a STRING.
 * 
 * WHITESPACE should always be omitted.
 *    COMMENTS should be stripped, and are defined as text occurring in:
 *    ;<TEXT>
 * 
 * Example file below.  Note that the second line is technically invalid.  
 * The event handler must decide whether this should be allowed.
 *    --FILE BELOW--
 *    [gaben]
 *    hi = clams
 *    bye = "NO CLAMS"
 *
 *    [valve]
 *    cannot
 *    maintain
 *    products
 */

The current behavior is expected.

Could you show me the "original" specification? (not the wiki) Also, could you show me a single implementation which allows spaces in the identifier?

afwn90cj93201nixr2e1re commented 4 years ago

https://github.com/compuphase/minIni https://github.com/SemaiCZE/inicpp/wiki/INI-format-specification

Here's no original spec, idk how i should rename PR. So, any parsers allow spaces in keys and values and token every time was =, for string where is = placed should be escaped with \, so sad asd=asdas\=d should be translated to -> key = "sad asd", value = "asdas=d".

afwn90cj93201nixr2e1re commented 4 years ago

From your spec's seems like keys also should be allowed as values but that's not true, values get off for "", but keys not:

изображение

Too much unexpected stuff.

Arkshine commented 4 years ago

I hear you. There is no need to generalize and dramatize everything, it's not helping. Showing random Github repositories with specific implementation does not mean everything is the same (it's not).

Implementation varies and I'm not against supporting spaces in keys "officially".

Documentation is indeed unclear about the quote around the key. It's likely unintended behavior. It's too late to make big changes on it. However, we can either:

  1. Update documentation about using " and remote_quotes for keys
  2. Update documentation about using " and strip quotes around keys automatically in core (like value)
  3. Adding an option to allow spaces in keys as it is (no need ")

I don't have a strong preference.

1) would be the safest. 2) Consistency with value 3) The most convenient

afwn90cj93201nixr2e1re commented 4 years ago

I think we should make some breaking changes in 1.10, or create 1.11 instead. So what do you think? #798

Showing random Github repositories with specific implementation does not mean everything is the same (it's not).

It's not random, i used this repos in past.