Issue 5828 - Additional paths require settings.ini

Issue 5828 - Additional paths require settings.ini

[Vasily Kuznetsov, 2019-01-04 18:46:03]

Hi Rosie,

Your approach works, but it introduces a side-effect of making the error message
about missing config harder to understand. I think there's a better way -- see
below.

Cheers,
Vasily

https://codereview.adblockplus.org/29972568/diff/29972569/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29972568/diff/29972569/cms/sources.py#newc...
cms/sources.py:79: return ConfigParser.SafeConfigParser()
This will do the trick, however, I'm worried that it makes the error message for
missing top level config file (that still should be required) less helpful. With
these change, what the user will see is:

    ... stackstrace ...
    ConfigParser.NoSectionError: No section: 'general'

whereas before it was:

    ... stackstrace ...
    IOError: [Errno 2] No such file or directory: 'docs/settings.ini'

Of course the old message is not very helpful either, but at least it was more
clear what file to look at to fix the problem. Now this is no longer the case
and more intimate knowledge of CMS would be required to understand what went
wrong.

I'm thinking that it would probably be better to change the code in
`create_source` instead. Something like this:

	-    config = source.read_config()
	     try:
	+        config = source.read_config()
		 ap = config.get('paths', 'additional-paths').strip()
		 additional_paths = filter(None, ap.split())
	-    except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
	+    except (ConfigParser.NoSectionError, ConfigParser.NoOptionError, IOError):
		 additional_paths = []

What do you think?

[rhowell, 2019-01-08 00:19:25]

Hey Vasily,

Thanks! Let me know if you see anything else?

https://codereview.adblockplus.org/29972568/diff/29972569/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29972568/diff/29972569/cms/sources.py#newc...
cms/sources.py:79: return ConfigParser.SafeConfigParser()
On 2019/01/04 18:46:03, Vasily Kuznetsov wrote:
> This will do the trick, however, I'm worried that it makes the error message
for
> missing top level config file (that still should be required) less helpful.
With
> these change, what the user will see is:
> 
>     ... stackstrace ...
>     ConfigParser.NoSectionError: No section: 'general'
> 
> whereas before it was:
> 
>     ... stackstrace ...
>     IOError: [Errno 2] No such file or directory: 'docs/settings.ini'
> 
> Of course the old message is not very helpful either, but at least it was more
> clear what file to look at to fix the problem. Now this is no longer the case
> and more intimate knowledge of CMS would be required to understand what went
> wrong.
> 
> I'm thinking that it would probably be better to change the code in
> `create_source` instead. Something like this:
> 
> 	-    config = source.read_config()
> 	     try:
> 	+        config = source.read_config()
> 		 ap = config.get('paths', 'additional-paths').strip()
> 		 additional_paths = filter(None, ap.split())
> 	-    except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
> 	+    except (ConfigParser.NoSectionError, ConfigParser.NoOptionError,
IOError):
> 		 additional_paths = []
> 
> What do you think?

Yeah, this seems clearer. Done.

[Vasily Kuznetsov, 2019-01-08 18:45:41]

LGTM

[rhowell, 2019-01-08 22:27:37]

On 2019/01/08 18:45:41, Vasily Kuznetsov wrote:
> LGTM

Thanks, I should be able to push the changes to the repo, but now tox is failing
because of an update to Pytest. It seems it's broken compatibility with
pytest-console-scripts, and now all of the tests in test_xtm_translate.py that
use `@pytest.mark.script_launch_mode('subprocess')` are failing. For a quick
fix, I can pin pytest to the previous version in tox.ini: pytest<=4.0.2. Should
we do that? And if so, should that change be included with this?