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

Fix errors during configuration update when reading obsolete options. #388

Merged
merged 3 commits into from Dec 18, 2014

Conversation

phw
Copy link
Member

@phw phw commented Dec 16, 2014

This fix makes old confiuration options, for which there is no Option instance in the code anymore, accessible.

Fixes PICARD-642.

@mwiencek: Since you reworked the parts of the code that finally led to this, you maybe have some better idea how to implement this.

Thix fix makes old confiuration options, for which there is no Option instance in the code anymore, accessible.

Fixes PICARD-642.
@phw phw force-pushed the PICARD-642-config-upgrade-error branch from 0f5125a to add62c5 Compare December 17, 2014 08:14
@phw
Copy link
Member Author

phw commented Dec 17, 2014

I have reproduced the issue and tested the fix on both Windows and Linux by using pre 1.0 versions of Picard to generate the configuration and then running the latest code.

For reference the commits that changed the option loading (and thus created this problem) were:
6ff1f53 (this just started the refactoring)
c50cdf7 (this finally prevents loading unregistered options)

@mwiencek
Copy link
Member

Looks good to me. My only concern would be if type.convert throws an exception for some reason. I can't think of any reason why it would, but the states peoples' configurations end up in always surprise me.

@mwiencek
Copy link
Member

I guess one example would be if the options don't exist for some reason and value is None or an empty string. Maybe it'd be safer to factor out lines 47-55 (which checks self.__config.contains) and use that as the basis for the new method.

@phw
Copy link
Member Author

phw commented Dec 17, 2014

Thanks for the feedback, I applied some refactoring. This really makes this code much better, the value method and the [] operator now share most code, the method is just more generic and doesn't use the option registry.

@mwiencek
Copy link
Member

Looks great 👍

@zas
Copy link
Collaborator

zas commented Dec 18, 2014

Can i merge it to master ?

@phw
Copy link
Member Author

phw commented Dec 18, 2014

It's ready for merging.

Can i merge it to master ?

Either that or merge it to the 1.3.1 branch and later merge that branch back to master.

zas added a commit that referenced this pull request Dec 18, 2014
Fix errors during configuration update when reading obsolete options.
@zas zas merged commit 0e43410 into metabrainz:master Dec 18, 2014
@phw phw deleted the PICARD-642-config-upgrade-error branch December 18, 2014 20:06
zas added a commit that referenced this pull request Dec 19, 2014
Fix errors during configuration update when reading obsolete options.
(cherry picked from commit 0e43410)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants