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

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

Open
afwn90cj93201nixr2e1re opened this issue Nov 23, 2019 · 18 comments
Open

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

afwn90cj93201nixr2e1re opened this issue Nov 23, 2019 · 18 comments

Comments

@afwn90cj93201nixr2e1re
Copy link
Contributor

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
Copy link
Member

Arkshine commented Nov 23, 2019

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
Copy link
Contributor Author

afwn90cj93201nixr2e1re commented Nov 23, 2019

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
Copy link
Member

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
Copy link
Contributor Author

afwn90cj93201nixr2e1re commented Nov 23, 2019

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@afwn90cj93201nixr2e1re
Copy link
Contributor Author

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

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

@afwn90cj93201nixr2e1re
Copy link
Contributor Author

@Arkshine
Copy link
Member

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

@afwn90cj93201nixr2e1re
Copy link
Contributor Author

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

@Arkshine
Copy link
Member

Arkshine commented Nov 23, 2019

Great. That's not what you said here though: #800 (comment)
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
Copy link
Contributor Author

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

@afwn90cj93201nixr2e1re afwn90cj93201nixr2e1re changed the title Provide custom token for INI_PARSERS Make ini parser format same as original. Nov 23, 2019
@Arkshine
Copy link
Member

Arkshine commented Nov 23, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@afwn90cj93201nixr2e1re afwn90cj93201nixr2e1re changed the title Make ini parser format same as original. Make ini parser format same as other normal formats. Nov 23, 2019
@Arkshine
Copy link
Member

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
Copy link
Contributor Author

afwn90cj93201nixr2e1re commented Nov 24, 2019

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.

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

No branches or pull requests

2 participants