ConfigUpdater.get returns string literal if fallback provided

[bdbenim]

Description of your problem

The output of the get() method is a different type depending on whether or not it returns the fallback option, meaning that it cannot be reliably assumed to be an object of type Option as expected.

import configupdater
conf = configupdater.ConfigUpdater()
conf.read("config.ini")
example1 = conf.get("section", "existingKey", "fallback").value
print(example1)
conf["section"].setdefault("nonexistantKey", "some value")
example2 = conf.get("section", "nonexistantKey", "fallback").value
print(example2)
example3 = conf.get("section", "nonexistantKey2", "fallback").value
print(example3)

Output:

value of key "existingKey"
some value
Traceback (most recent call last):
  File "example.py", line 9, in <module>
    example3 = conf.get("section", "nonexistantKey2", "fallback").value
AttributeError: 'str' object has no attribute 'value'

Please provide any additional information below.
According to this documentation, the get method should return an object of type Option. However, if a fallback value is provided, the literal value of this fallback is returned. I would expect the behaviour of examples 2 and 3 to be the same, where if I provide a fallback value then it will be treated as an option in the respective section and returned as such. The ability to provide a fallback is pointless since I still have to check the output and do something different depending on whether or not the key was found in that section

Versions and main components

• ConfigUpdater Version: 3.1.1
• Python Version: 3.10.12
• Operating system: Ubuntu 22.04
• How did you install ConfigUpdater: pip

[FlorianWilhelm]

Thanks for posting this issue @bdbenim. This behaviour is really inconsistent. I think there are two possibilities. Either we do not allow passing a default option (and by this being not consistent to the .get of ConfigParser ) or we implement it the way you suggested, so that we return always an Option.

The latter alternative makes more sense to me. What do you think @abravalheri?

@bdbenim Would you be interested in providing a PR to fix this bug?

[abravalheri]

The documentation seems to be outdated. The signature, however, shows the correct information:

     @overload  # type: ignore[override] 
     def get(self, section: str, option: str) -> Option: 
         ... 
  
     @overload 
     def get(self, section: str, option: str, fallback: T) -> Union[Option, T]: 
         ...

I belive we have 3 options here:

A. Fix the docs to match the signatures.
B. Change it as suggested to make the users' life easier.
C. Introduce a new keyword argument.

The problem with (B) is that it would be a backward incompatible change, so we have to bump the version and it may break users existing code.

I suppose that even if we just do (A) users life are not that complicated as they can always write:

example = conf.get("section", "nonexistant",  fallback=Option("nonexistant", value="2"))

(A bit verbose, but explicit).

I am OK either way, but if we go for (B), it would be nice to check around github/grep.app to see if it would not break anyone's code.

[bdbenim]

@FlorianWilhelm I wouldn't mind taking a shot at fixing it and providing a PR.

@abravalheri I see what you mean, and I agree that option (B) is potentially a breaking change. I'm trying think of the edge cases in my mind, and I suspect that it might not actually break anything, since any call to get should be expecting an Option to be returned, and is just prepared to potentially handle some other type. So if it's modified to always return an option, that should still work. Though as I'm typing this I realize that someone might be using the return type to determine whether or not the key was found in the config, which would indeed be broken by this change.

I'm of course selfishly inclined toward option (B), but option (C) I think would satisfy me without breaking anyone else's code. Something like:

     @overload  # type: ignore[override] 
     def get(self, section: str, option: str) -> Option: 
         ... 
  
     @overload 
     def get(self, section: str, option: str, fallback: T) -> Union[Option, T]: 
         ...

    @overload 
     def get(self, section: str, option: str, fallbackLiteral: T) -> Option: 
         ...

The third version could return Option(option, fallbackLiteral) if the option isn't found in the section. Thoughts?

[bdbenim]

I gave it a shot in #125 and it seems to work. I couldn't get the unit tests to run in my development environment, but did some testing of my own in a script. The existing behaviour is unchanged as far as I can tell, but if the new parameter returnOption is set to True then the get method will return an Option object. Let me know what you think, and I'd be happy to make modifications if necessary.

[FlorianWilhelm]

Thanks for the PR @bdenim and thanks for explaining the different alternatives @abravalheri.

I am not quite sure if the alternative C doesn't make the API too complicated as the return type now depends on a true/false flag. For me the simplest solution is B, which has the downside of breaking the API and we would need to jump to v4, which I guess is fine and configupdater users are power users anyway and hopefully apply semantic versioning. I think alternative A is also not bad (didn't know this was possible) as it solves the problem even though it is cumbersome and one would only need to fix the docs and maybe add this example to the docs because it might not be obvious.

In summery, my preference is B > A > C. But @abravalheri has the final word on this :-)

[abravalheri]

Sorry for the delay guys.

To be honest my preference is A > C > B 😅, mainly because of the semantics of the fallback term in the Python ecosystem. My reasoning is the following:

My impression when I read fallback is that it represents the "stand-in" for the thing you are trying to get. When that thing does not exist, the fallback will be returned. In most Python code that is the semantics attached to this term. I honestly did not came across yet with any counter example (a rough look in grep.app also reinforce this impression).

The proposal to use fallback to fill the value for the option that will be created in the case no option exists, don't quite match these semantics in my opinion.
It would be more of a value_to_populate_fallback_option kind of thing, which I cannot summarise in a single word.

(I am not a native speaker, so this opinion, of course, has to be taken with a grain of salt).

The other problem is: how do we know that the majority of users want to get Option("option", None)
when they write configupdater.get("section", "option", fallback=None) and not simply None?
In this case, simply returning None may be more obvious for a lot of users...

I think option A sidesteps the difficulties.
Sure, configupdater.get("section", "option", fallback=Option("option", None)) is more verbose to write, but it works and is obvious for the reader to find out the intention of the code.

I don't want to strong arm anyone, so for now I will simply write a PR to correct the docs, but we can leave this discussion open until we reach a consensus.

Hopefully #127 improves the text. Please feel free to add suggestions directly to the PR.

---

P.S.: Most of this problem derives from the fact that the original ConfigParser object can afford to be a glorified Mapping[str, Mapping[str, str]], unfortunately ConfigUpdater cannot afford that. It needs another extra nesting level in the data structure... It is unfortunate and might be a bit difficult to wrap the head around at first, but that is what allow us to process visual information and comments...