search

CampbellGroup / common

Shared campbell lab code.
GNU Lesser General Public License v3.0
8 stars 5 forks source link

Try/Except config loading behavior is unexpected if custom config has import errors. #185

Open jayich opened 7 years ago

jayich commented 7 years ago

The point of the Try/Except statement for loading config files is to load a custom configuration file if a user has one, and if not then it will load a default configuration file from common.

If you accidentally break a custom config file in such a way that it won't import, the Try/Except logic we employ will give unexpected behavior, it will load the common config file, even though the user has as custom config file.

This is a bit subtle, and not what I think was the original intent of the Try/Except config import code. We should consider modifying this logic to raise an error if the import fails because of errors in a custom config file.

jchristensen133 commented 7 years ago

I agree it's not so useful and I've been burned by this before. I stopped using the try/except and just have the code break if the import fails. If you think about it, whatever you're doing relies on the custom config you're trying to implement anyway and having a default doesn't really help you.

aransfor commented 7 years ago

The point of the try except is for new users so the clients work. While I agree with you guys I am sensitive to first time installers. But it's up to you guys if we keep it

jayich commented 7 years ago

I agree with @aransfor, clients should work out of the box with a default config. The wavemeter client as a good example.

We can clean this behavior up so that we get good default behavior when no custom config file is present, and an error is thrown when there is a custom config, but the custom config has an error.