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

Allow handler to be called on a key without '=' or ':' #87

Merged
merged 5 commits into from
Oct 5, 2019
Merged

Allow handler to be called on a key without '=' or ':' #87

merged 5 commits into from
Oct 5, 2019

Conversation

weltling
Copy link
Contributor

@weltling weltling commented Oct 3, 2019

The actual need on this is as follows. The INI file contains a special section containing all the sections in the file. That is used to validate the INI file itself. Eg

[list all]
section0
section1

[section0]
key=val

[section1]
key=val

For this to work, the keys in the list all section need to be passed to the handler, so they can be recorded and used later for the validation. This has been made configurable, so won't affect the default library behavior.

Thanks.

@benhoyt
Copy link
Owner

benhoyt commented Oct 3, 2019

Hi @weltling, this is reasonable request - however, inih already has a ton of #if's and I'm a bit hesitant to add yet another one. Do you have control over your INI file? If so, could you simply add = signs with a blank value so it parses? For example:

[list all]
section0=
section1=

@weltling
Copy link
Contributor Author

weltling commented Oct 3, 2019

Hi @benhoyt, thanks for checking the PR.

Leaving aside that changing the INI spec for the particular project would likely have to go through some cycles, another part of the issue is that the file is used by different parties and at least one already uses a parser that supports this kind of behavior. I fully understand your point to not to pollute inih with #if's, so ofc it's up to you. I'll have to check, whether carrying teh patch would be a solution, or come up with something else. In any case, thanks for your work and a useful piece of software :)

Thanks.

@benhoyt
Copy link
Owner

benhoyt commented Oct 3, 2019

Cool, let me know what you find and comment back here if you can't work around it.

@benhoyt benhoyt closed this Oct 3, 2019
@bluca
Copy link

bluca commented Oct 4, 2019

@benhoyt would unconditionally allow value-less keys be an option? That way there's no extra ifdefs. There are precedents in other ini parsers support this.
Thanks!

@benhoyt benhoyt reopened this Oct 4, 2019
@benhoyt
Copy link
Owner

benhoyt commented Oct 4, 2019

Okay - I've looked at this a bit further and I'm reopening. I don't want to enable this by default, but I'm willing to add the #if version. Python's configparser (which inih is somewhat modeled after) flags this as an error by default too, but has an allow_no_value option to enable this mode. I'll request a couple of tweaks to the PR and the addition of tests -- reviewing now.

Copy link
Owner

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments. Two other things:

  1. Please add some snapshot tests for this to tests/ and tests/unittest.[sh|bat]. I don't think there are actually tests for the current "error on no value" case, so be good to add that at the same time.
  2. Please use 4-space tabs rather than actual tab characters to match the rest of the file.

ini.c Outdated
@@ -202,7 +202,12 @@ int ini_parse_stream(ini_reader reader, void* stream, ini_handler handler,
}
else if (!error) {
/* No '=' or ':' found on name[=:]value line */
error = lineno;
#if INI_ALLOW_KEY_WITHOUT_EQUAL
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this INI_ALLOW_NO_VALUE to match Python's "allow_no_value" option.

ini.c Outdated
#if INI_ALLOW_KEY_WITHOUT_EQUAL
*end = '\0';
name = rstrip(start);
if (!HANDLER(user, section, name, NULL))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add a && !error here like the other HANDLER calls so that it returns the first error.

ini.c Outdated
name = rstrip(start);
if (!HANDLER(user, section, name, NULL))
#endif
error = lineno;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clever :-) but the indentation not matching up is a bit weird. Can we do this instead?

#if INI_ALLOW_NO_VALUE
        *end = '\0';
        name = rstrip(start);
        if (!HANDLER(user, section, name, NULL))
            error = lineno;
#else
        error = lineno;
#endif

ini.h Outdated
@@ -134,6 +134,13 @@ int ini_parse_string(const char* string, ini_handler handler, void* user);
#define INI_CALL_HANDLER_ON_NEW_SECTION 0
#endif

/* If a section contains a key without '=' or ':' signs, it would be not
passed to the handler. Setting the below to 1 will still call the handler
and explicitly set value to NULL. */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change the wording here to match the style of the other ones:

/* Nonzero to allow a name without a value (no '=' or ':' on the line) and
   call the handler with value NULL in this case. Default is to treat
   no-value lines as an error. */

@benhoyt benhoyt changed the title Allow handler to be called on a key withouth '=' or ':' Allow handler to be called on a key without '=' or ':' Oct 4, 2019
@weltling
Copy link
Contributor Author

weltling commented Oct 4, 2019

@benhoyt glad you change your mind :) I'm working on some tests, will update next days.

Thanks.

@weltling
Copy link
Contributor Author

weltling commented Oct 5, 2019

@benhoyt ready for the next round, I think. Please let me know, if something is missing.

Thanks.

Copy link
Owner

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks! Just a few comments.


gcc ../ini.c -DINI_ALLOW_NO_VALUE=1 unittest.c -o unittest_call_allow_no_value
./unittest_call_allow_no_value > baseline_call_allow_no_value.txt
rm -f unittest_call_allow_no_value
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and the committed .txt file shouldn't have the word "call" in it. Should be called just unittest_allow_no_value.

tests/unittest.c Show resolved Hide resolved
tests/baseline_stop_on_first_error.txt Show resolved Hide resolved
@weltling
Copy link
Contributor Author

weltling commented Oct 5, 2019

Everything should be resolved now. Thanks for the quick check.

Copy link
Owner

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@benhoyt
Copy link
Owner

benhoyt commented Oct 5, 2019

Thanks for this -- I'll merge and release a new version shortly.

@benhoyt benhoyt merged commit 82fdde3 into benhoyt:master Oct 5, 2019
@weltling
Copy link
Contributor Author

weltling commented Oct 5, 2019

Thanks a lot!

@benhoyt
Copy link
Owner

benhoyt commented Oct 5, 2019

Released r46 with this feature: https://github.com/benhoyt/inih/releases/tag/r46 -- thanks!

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.

3 participants