confuse icon indicating copy to clipboard operation
confuse copied to clipboard

Bug with template OneOf()

Open mozesa opened this issue 4 years ago • 2 comments

Dear Adrian,

first of all I would like to say Thank you for this awesome package. It is really great.

Maybe I was not enough thorough but 'import_write': confuse.OneOf([bool, 'ask', 'skip']), allows me to use such a values like import_write: asdfasdfasdf in config_default.yaml.

The example is the example of package so you can reproduce it by yourself.

What do I do wrongly?

Thanks for your help in advance.

mozesa avatar May 12 '20 08:05 mozesa

Hmm; thanks for pointing this out. Here's a short little script to demonstrate the problem:

import confuse
config = confuse.Configuration('foo')
config.add({ 'import_write': 'qux' })
print(config['import_write'].get(confuse.OneOf(['ask', 'skip'])))

Looking at the code, I have realized that I have no idea how OneOf is supposed to work. It was introduced in https://github.com/beetbox/confuse/commit/473bc036 and it has had several strange things from the beginning. The salient weirdness is this exception handler: https://github.com/beetbox/confuse/blob/3bf9680e7d242136cc304232f902db06c4cc8e11/confuse.py#L1392-L1393

I don't think we should be handling that root exception at all, but silently passing seems like it is certainly a problem. Other weird things include this mutable update when trying to access a value: https://github.com/beetbox/confuse/blob/3bf9680e7d242136cc304232f902db06c4cc8e11/confuse.py#L1363

I don't really understand what that template argument is for.

Finally, this special handling of filenames in OneOf: https://github.com/beetbox/confuse/blob/3bf9680e7d242136cc304232f902db06c4cc8e11/confuse.py#L1374-L1375

seems pretty weird—why would these two templates need to interact in a special way?

Anyway, it seems like OneOf might need to be rethought from the ground up. (And of course we should add a test case to catch this bug!)

sampsyo avatar May 12 '20 12:05 sampsyo

Hi, I got to this issue while searching for better usage examples of confuse.OneOf() and templates. I was sure I was doing something wrong. I tested by changing a config value to a non-Integer, and tested using confuse.Integer(): It worked. So I am here to confirm that OneOf() is indeed not working, but I got a day or two too late.

PS: This library is awesome, btw. My project Hume will benefit a lot from it. Thanks.

buanzo avatar May 17 '20 13:05 buanzo