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

Post Processing config saving select-list values incorrectly #5165

Merged
merged 6 commits into from
Sep 9, 2018

Conversation

sharkykh
Copy link
Contributor

@sharkykh sharkykh commented Sep 8, 2018

Fixes #5155

The tests for select-list are how they should be, however the components needs to be changed to allow for them to pass, and it cannot be changed without changing the usages of it, which will cause an infinite loop at the moment...

@sharkykh sharkykh mentioned this pull request Sep 8, 2018
8 tasks
@sharkykh sharkykh removed the Changelog Requires a changelog entry label Sep 8, 2018
OmgImAlexis
OmgImAlexis previously approved these changes Sep 9, 2018
@sharkykh
Copy link
Contributor Author

sharkykh commented Sep 9, 2018

Alright, made the changes to do this in backend. This is actually better since ast.literal_eval is safe, and it will automatically fix the issue without having to ask the users to save their config again.

@@ -95,6 +95,34 @@
logger = logging.getLogger(__name__)


def fix_incorrect_list_values(data):
Copy link
Contributor

@p0psicles p0psicles Sep 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put a method in medusa/config.py.
And just fix it for the suspected fields there.
Then call the method from main somewhere here:
https://github.com/pymedusa/Medusa/blob/release/hotfix-0.2.10/medusa/__main__.py#L1119

That way we don't clutter __main__.py more then necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought since we are removing this in a future version it would be best to put it all in one place?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's technically not a config migration. But it's easy to reuse the self.config_obj property. So maybe just add a function there named fix_config() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's really good either way

@medariox medariox merged commit f0cec79 into develop Sep 9, 2018
@medariox medariox deleted the bugfix/post-processing-sync-files branch September 9, 2018 19:10
sharkykh added a commit that referenced this pull request Sep 9, 2018
* Fix config-post-processing

* Add failing tests for select-list

* Fix Lint

* Update changelog

* Move fix to backend
@sharkykh sharkykh mentioned this pull request Nov 11, 2018
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants