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

Add support for "global" properties (before any section)? #123

Open
Delgan opened this issue Oct 5, 2023 · 5 comments
Open

Add support for "global" properties (before any section)? #123

Delgan opened this issue Oct 5, 2023 · 5 comments

Comments

@Delgan
Copy link
Contributor

Delgan commented Oct 5, 2023

Describe your use-case

Hi again.

I implemented a library on top of configupdater and someone reported it didn't work with EditorConfig configuration files. This is because such file can contain "global" properties which declared before any section:

root = true

[section]
foo = bar
baz = 42

There are no definitive specifications on the acceptable syntax of an INI file. In that respect, configupdater is consistent with configparser since both reject such file and raise MissingSectionHeaderError error.

Given that there are certain variants of the INI format that accept global parameters (also Apache), what do you think about extending configupdater support to such files?

Describe the solution you would like to see for your use-case

I imagine a new option to the ConfigUpdater class, such as allow_global_parameters defaulting to False. If True, the global parameters would be added to a section with None name or something like that.

I can try to open a PR if you're interested with such feature (don't know how technically feasible it is yet).

Otherwise, I'll just hack a workaround in my own library. But since it may be beneficial to others, I wanted to discuss it upstream first.

@FlorianWilhelm
Copy link
Member

Hi @Delgan, thanks for posting this feature request.

I am quite hesitant to make ConfigUpdater more powerful than ConfigParser when it comes to the definition of what an INI file really is. So far we sticked to a strict "being consistent with ConfigParser as much as possible"-rule.

Breaking this rule, could increase the complexity of ConfigUpdater's code too much and also our work in maintaining it. For those reasons I would rather refrain from having a allow_global_parameters option as long as ConfigParser doesn't have it.
What do you think @abravalheri?

Maybe you can describe your workaround here so that users having the same problem easily find it. Do you prepend a section with a unique name to the file when reading it? Or first parse the file to find the first option and then insert a section above? Could you post a small code snippet for others here?

@Delgan
Copy link
Contributor Author

Delgan commented Oct 8, 2023

Thank you for having considered my request.
I can understand that this is out-of-scope for this library.

I have not implemented any workaround yet.
Some solutions are listed in this StackOverflow question: Using ConfigParser to read a file without section name.

This will likely requires adding a fake section on top of the INI file content. To do it right, care must also be taken not to cause DuplicateSectionError.

@Delgan
Copy link
Contributor Author

Delgan commented Nov 4, 2023

Small update, I just discovered CPython has an open PR for this exact feature since 6 years: python/cpython#2735...
It's not clear why it hasn't been merged yet, despite numerous discussions.

I think a lot of users would be happy to use ConfigUpdater for this feature, without waiting indefinitely for the standard library to implement it.

@FlorianWilhelm
Copy link
Member

FlorianWilhelm commented Nov 9, 2023

Thanks @Delgan for pointing this out. I see the point that the userbase of ConfigUpdater might increase by supporting this feature and maybe even a lot of users that would be happy with ConfigParser having global properites would switch to ConfigUpdater just for that. The thing is that growing the userbase is not really our goal, rather keeping the project focused and easy to maintain because @abravalheri and I only work at our spare time on it. So maybe there is a good reason that the CPython ConfigParser implementation never went for supporting "global" properties? I thought a bit how I would implement this feature consistently, i.e. not just a small hack, in ConfigUpdater and it would change not just a few things but rather a lot. I mean, how would you access those global parameters? How would you deal with them in our current 2-layer approach that allows only key/values with sections, i.e. in the 2-layer. Also a lot of new edge cases would come up and even though what ConfigUpdater does may sound simple, we had tons of edge cases fixed over the years. Also we could not rely on ConfigParser any more to do the final check in order to see if we are still compliant, ConfigParser is our gold standard right now.

Considering all this, I really think the best way for you would be to write a little wrapper library for ConfigUpdater that uses some kind of hack, e.g. prepend a "[global]" section, use the functionality of ConfigUpdater and delete "[global]" afterwards. I am sorry but we really have to think long term here and supporting a feature like this directly within ConfigUpdater will just cause too much work on our side. I hope you understand.

@Delgan
Copy link
Contributor Author

Delgan commented Nov 9, 2023

Sorry for pushing, and thanks for the time you took to consider the technical implications.

I mean, how would you access those global parameters?

One possibility would be to use a sentinel value to replace the missing section name. This is what they implemented in the PR I linked.

Also we could not rely on ConfigParser any more to do the final check in order to see if we are still compliant, ConfigParser is our gold standard right now.

Yes, I totally agree and it makes a lot of sense. This is a very natural way to define the scope of this library.

Considering all this, I really think the best way for you would be to write a little wrapper library for ConfigUpdater that uses some kind of hack, e.g. prepend a "[global]" section, use the functionality of ConfigUpdater and delete "[global]" afterwards.

I want to do it right, therefore it'll probably look something like this:

updater = ConfigUpdater()
try:
    updater.read_string(cfg)
except MissingSectionHeaderError:
    i = 1
    while True:
        dummy_section = f"[dummy-section-{i}]"
        if dummy_section in cfg:
            i += 1
            continue
        return updater.read_string(f"{dummy_section}\n{cfg}")

The dummy session must also be removed after write().

Not very elegant nor straightforward, but I can live with it. :)

I am sorry but we really have to think long term here and supporting a feature like this directly within ConfigUpdater will just cause too much work on our side. I hope you understand.

Yeah no problem. It wasn't clear to me whether there was a small chance that such a feature would be accepted or not.
Thanks for the clarification. I'm now going to implement the solution described above. If such a feature is finally added to the standard configparser, we could think about re-opening the ticket. ;)

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