PR: Allow values with '%' in config system

[dalthviz]

Fixes #4921

[jitseniesen]

I am not sure this actually fixes #4921. It does prevent the error when the user enters %load_ext autoreload, %autoreload 2 by removing the % characters when saving the config, but when the config value is loaded the % characters are not added back in so the value is now load_ext autoreload, autoreload 2.

[dalthviz]

Yep, you are right @jitseniesen, it doesn't restore the %, maybe should I try to replace the % with something else and then restore it by replacing again? or maybe created a new line edit only for magics and add the % to each entry? What do you think @ccordoba12 ?

Although with or without the % the magic is executed.

[ccordoba12]

it doesn't restore the %, maybe should I try to replace the % with something else and then restore it by replacing again?

I agree with this idea. I think it's much simpler than adding a lineedit only for magics.

@dalthviz, please implement this and add a test for it.

[pep8speaks]

Hello @dalthviz! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 26, 2017 at 04:33 Hours UTC

[jitseniesen]

An alternative would be to find out why our config system does not allow for % to be saved. I remember I had a quick look once and the fact that we use RawConfigParser should allow us to use % characters. However, at one point in the code this does not work; I forget where and I could not immediately find out why.

This may be more work, but replacing % with another character like ¿ caused problems if the user uses the second character somewhere in the configuration (admittedly pretty unlikely). It might also cause issues with how to encode that character, especially in Python 2, though the tests pass so maybe it's okay (or maybe you did not add a test).

[dalthviz]

Totally right @jitseniesen. Testing a little bit and reading the docs the problem was in Python 3 since, for ConfigParser, there is a default value for a keyword argument interpolation that is BasicInterpolation

[ccordoba12]

The solution is sweet and simple. Thanks @jitseniesen for your suggestion!

[jitseniesen]

It's @dalthviz who found the solution; I could not find it when I looked for it.

[dalthviz]

Thanks for the comments @jitseniesen, they really helped us to gain more perspective in the problem and helped me to find this nice solution!