From 58f031630c6b04c0178f268bf5ad3803455d0c31 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Fri, 22 May 2015 14:44:04 +0200 Subject: [PATCH 01/19] user-stylesheet can be read from relative paths This ist just a first draft to approach issue622 (https://github.com/The-Compiler/qutebrowser/issues/622) and my very first babysteps with python. With this change it is possible to set a user-stylesheet with a relative path, eg.: :set ui user-stylesheet mystyle.css where mystyle.css is in the ~/.config/qutebrowser/. --- .gitignore | 2 ++ qutebrowser/config/configtypes.py | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/.gitignore b/.gitignore index f3ff3652a..9552dc3c4 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,5 @@ __pycache__ /htmlcov /.tox /testresults.html +tags +*.swp diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index b9c760116..354c09598 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -34,6 +34,7 @@ from PyQt5.QtWidgets import QTabWidget, QTabBar from qutebrowser.commands import cmdutils from qutebrowser.config import configexc +from qutebrowser.utils import standarddir SYSTEM_PROXY = object() # Return value for Proxy type @@ -792,6 +793,9 @@ class RegexList(List): raise configexc.ValidationError(value, "items may not be empty!") +# TODO(lamar) Issue622, relative paths for some config files and directories +# should be implemented here in the base class for files and below in the base +# class for directories. class File(BaseType): """A file on the local filesystem.""" @@ -806,6 +810,10 @@ class File(BaseType): raise configexc.ValidationError(value, "may not be empty!") value = os.path.expanduser(value) try: + if not os.path.isabs(value): + relpath = os.path.join(standarddir.config(), value) + if os.path.isfile(relpath): + value = relpath if not os.path.isfile(value): raise configexc.ValidationError(value, "must be a valid file!") if not os.path.isabs(value): @@ -1160,6 +1168,10 @@ class UserStyleSheet(File): value = os.path.expandvars(value) value = os.path.expanduser(value) try: + if not os.path.isabs(value): + relpath = os.path.join(standarddir.config(), value) + if os.path.isfile(relpath): + value = relpath if not os.path.isabs(value): # probably a CSS, so we don't handle it as filename. # FIXME We just try if it is encodable, maybe we should From 29b25206f6341f8747447e451ddda78e9bf925fd Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Fri, 22 May 2015 17:21:00 +0200 Subject: [PATCH 02/19] Fix UserStyleSheet, roll back File The former version of UserStyleSheet never actually loaded the css file, this is now fixed. The changes to class File were rolled back as its functions are overloaded by UserStyleSheet; a general solution in classes File and Directory can be implemented when the changes in UserStyleSheet meet the expectation. --- qutebrowser/config/configtypes.py | 54 ++++++++++++++----------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 354c09598..60bd5ecce 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -793,15 +793,17 @@ class RegexList(List): raise configexc.ValidationError(value, "items may not be empty!") -# TODO(lamar) Issue622, relative paths for some config files and directories -# should be implemented here in the base class for files and below in the base -# class for directories. class File(BaseType): """A file on the local filesystem.""" typestr = 'file' + def transform(self, value): + if not value: + return None + return os.path.expanduser(value) + def validate(self, value): if not value: if self._none_ok: @@ -810,10 +812,6 @@ class File(BaseType): raise configexc.ValidationError(value, "may not be empty!") value = os.path.expanduser(value) try: - if not os.path.isabs(value): - relpath = os.path.join(standarddir.config(), value) - if os.path.isfile(relpath): - value = relpath if not os.path.isfile(value): raise configexc.ValidationError(value, "must be a valid file!") if not os.path.isabs(value): @@ -822,11 +820,6 @@ class File(BaseType): except UnicodeEncodeError as e: raise configexc.ValidationError(value, e) - def transform(self, value): - if not value: - return None - return os.path.expanduser(value) - class Directory(BaseType): @@ -1159,19 +1152,33 @@ class UserStyleSheet(File): def __init__(self): super().__init__(none_ok=True) + def relapath(self, path): + if not os.path.isabs(path): + abspath = os.path.join(standarddir.config(), path) + if os.path.isfile(abspath): + return abspath + return path + + def transform(self, value): + path = os.path.expandvars(value) + path = os.path.expanduser(path) + path = self.relapath(path) + if not value: + return None + elif os.path.isabs(path): + return QUrl.fromLocalFile(path) + else: + data = base64.b64encode(value.encode('utf-8')).decode('ascii') + return QUrl("data:text/css;charset=utf-8;base64,{}".format(data)) + def validate(self, value): if not value: if self._none_ok: return else: raise configexc.ValidationError(value, "may not be empty!") - value = os.path.expandvars(value) - value = os.path.expanduser(value) + value = self.relapath(value) try: - if not os.path.isabs(value): - relpath = os.path.join(standarddir.config(), value) - if os.path.isfile(relpath): - value = relpath if not os.path.isabs(value): # probably a CSS, so we don't handle it as filename. # FIXME We just try if it is encodable, maybe we should @@ -1187,17 +1194,6 @@ class UserStyleSheet(File): except UnicodeEncodeError as e: raise configexc.ValidationError(value, e) - def transform(self, value): - path = os.path.expandvars(value) - path = os.path.expanduser(path) - if not value: - return None - elif os.path.isabs(path): - return QUrl.fromLocalFile(path) - else: - data = base64.b64encode(value.encode('utf-8')).decode('ascii') - return QUrl("data:text/css;charset=utf-8;base64,{}".format(data)) - class AutoSearch(BaseType): From 14ba20670b8923a480c460e41562561556d114d0 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Fri, 22 May 2015 17:31:37 +0200 Subject: [PATCH 03/19] Fix potential bug with missing path-expansion The last commit removed two lines in function validate of class UserStyleSheet that were expanding the path. As it turns out those two lines are needed by validate as well as transform, so I outsourced them to the function they both call at that point. --- qutebrowser/config/configtypes.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 60bd5ecce..a1cf77561 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1153,6 +1153,8 @@ class UserStyleSheet(File): super().__init__(none_ok=True) def relapath(self, path): + path = os.path.expandvars(path) + path = os.path.expanduser(path) if not os.path.isabs(path): abspath = os.path.join(standarddir.config(), path) if os.path.isfile(abspath): @@ -1160,9 +1162,7 @@ class UserStyleSheet(File): return path def transform(self, value): - path = os.path.expandvars(value) - path = os.path.expanduser(path) - path = self.relapath(path) + path = self.relapath(value) if not value: return None elif os.path.isabs(path): From 61f32b3e9b693895f2fad4f44122bcb6bb161dba Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Fri, 22 May 2015 18:40:56 +0200 Subject: [PATCH 04/19] Revert some changes, trying to get rid of the tox failures --- qutebrowser/config/configtypes.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index a1cf77561..a9d73d454 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1152,20 +1152,16 @@ class UserStyleSheet(File): def __init__(self): super().__init__(none_ok=True) - def relapath(self, path): - path = os.path.expandvars(path) + def transform(self, value): + if not value: + return None + path = os.path.expandvars(value) path = os.path.expanduser(path) if not os.path.isabs(path): abspath = os.path.join(standarddir.config(), path) if os.path.isfile(abspath): - return abspath - return path - - def transform(self, value): - path = self.relapath(value) - if not value: - return None - elif os.path.isabs(path): + path = abspath + if os.path.isabs(path): return QUrl.fromLocalFile(path) else: data = base64.b64encode(value.encode('utf-8')).decode('ascii') @@ -1177,7 +1173,12 @@ class UserStyleSheet(File): return else: raise configexc.ValidationError(value, "may not be empty!") - value = self.relapath(value) + value = os.path.expandvars(value) + value = os.path.expanduser(value) + if not os.path.isabs(value): + abspath = os.path.join(standarddir.config(), value) + if os.path.isfile(abspath): + value = abspath try: if not os.path.isabs(value): # probably a CSS, so we don't handle it as filename. From 93b92f4aab777ff14d74980e4d871ad5097ddb54 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Sat, 23 May 2015 16:09:44 +0200 Subject: [PATCH 05/19] Fix tox failure regarding exceptions in transform Function transform is not supposed to raise exceptions, so I wrapped the call to os.path.join in an if-clause to test if standarddir.config returns a valid value. --- qutebrowser/config/configtypes.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index a9d73d454..2941f6c72 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1158,9 +1158,10 @@ class UserStyleSheet(File): path = os.path.expandvars(value) path = os.path.expanduser(path) if not os.path.isabs(path): - abspath = os.path.join(standarddir.config(), path) - if os.path.isfile(abspath): - path = abspath + if standarddir.config(): + abspath = os.path.join(standarddir.config(), path) + if os.path.isfile(abspath): + path = abspath if os.path.isabs(path): return QUrl.fromLocalFile(path) else: From ad7920dda1bb110cda9983fb9ab6b3861caddf45 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Sat, 23 May 2015 16:49:40 +0200 Subject: [PATCH 06/19] Fix bug; all tox tests succeed My logic in the validate function of class UserStyleSheet was faulty and caused the check for encoding to be skipped. This is now fixed and all tests run successfully. --- qutebrowser/config/configtypes.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 2941f6c72..fb51a4c2e 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1176,12 +1176,11 @@ class UserStyleSheet(File): raise configexc.ValidationError(value, "may not be empty!") value = os.path.expandvars(value) value = os.path.expanduser(value) - if not os.path.isabs(value): - abspath = os.path.join(standarddir.config(), value) - if os.path.isfile(abspath): - value = abspath try: if not os.path.isabs(value): + abspath = os.path.join(standarddir.config(), value) + if os.path.isfile(abspath): + return # probably a CSS, so we don't handle it as filename. # FIXME We just try if it is encodable, maybe we should # validate CSS? From c54c637ccc65f4f114f3c25e960deac47b69ad6d Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Tue, 26 May 2015 12:38:04 +0200 Subject: [PATCH 07/19] Class File not transforms relative paths The code from function transform in class UserStyleSheet is now migrated to class File. --- qutebrowser/config/configtypes.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index fb51a4c2e..d0d822a6f 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -802,7 +802,13 @@ class File(BaseType): def transform(self, value): if not value: return None - return os.path.expanduser(value) + value = os.path.expanduser(value) + if not os.path.isabs(value): + if standarddir.config(): + abspath = os.path.join(standarddir.config(), value) + if os.path.isfile(abspath): + return abspath + return value def validate(self, value): if not value: @@ -1155,13 +1161,8 @@ class UserStyleSheet(File): def transform(self, value): if not value: return None - path = os.path.expandvars(value) - path = os.path.expanduser(path) - if not os.path.isabs(path): - if standarddir.config(): - abspath = os.path.join(standarddir.config(), path) - if os.path.isfile(abspath): - path = abspath + path = super().transform(value) + path = os.path.expandvars(path) if os.path.isabs(path): return QUrl.fromLocalFile(path) else: From f1129460d871d4bca46036085f8d869ff913ad49 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Tue, 26 May 2015 13:54:27 +0200 Subject: [PATCH 08/19] Class File now validates relative paths The code from function validate in class UserStyleSheet has been migrated to class File. One test had to be modified due to different expected behaviour. --- qutebrowser/config/configtypes.py | 34 ++++++++++++++++--------------- tests/config/test_configtypes.py | 3 +-- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index d0d822a6f..4b0b5b70f 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -818,6 +818,10 @@ class File(BaseType): raise configexc.ValidationError(value, "may not be empty!") value = os.path.expanduser(value) try: + if not os.path.isabs(value): + abspath = os.path.join(standarddir.config(), value) + if os.path.isfile(abspath): + return if not os.path.isfile(value): raise configexc.ValidationError(value, "must be a valid file!") if not os.path.isabs(value): @@ -1178,23 +1182,21 @@ class UserStyleSheet(File): value = os.path.expandvars(value) value = os.path.expanduser(value) try: - if not os.path.isabs(value): - abspath = os.path.join(standarddir.config(), value) - if os.path.isfile(abspath): + super().validate(value) + except configexc.ValidationError: + try: + if not os.path.isabs(value): + # probably a CSS, so we don't handle it as filename. + # FIXME We just try if it is encodable, maybe we should + # validate CSS? + # https://github.com/The-Compiler/qutebrowser/issues/115 + try: + value.encode('utf-8') + except UnicodeEncodeError as e: + raise configexc.ValidationError(value, str(e)) return - # probably a CSS, so we don't handle it as filename. - # FIXME We just try if it is encodable, maybe we should - # validate CSS? - # https://github.com/The-Compiler/qutebrowser/issues/115 - try: - value.encode('utf-8') - except UnicodeEncodeError as e: - raise configexc.ValidationError(value, str(e)) - return - elif not os.path.isfile(value): - raise configexc.ValidationError(value, "must be a valid file!") - except UnicodeEncodeError as e: - raise configexc.ValidationError(value, e) + except UnicodeEncodeError as e: + raise configexc.ValidationError(value, e) class AutoSearch(BaseType): diff --git a/tests/config/test_configtypes.py b/tests/config/test_configtypes.py index 871b6cf4d..c515c6cef 100644 --- a/tests/config/test_configtypes.py +++ b/tests/config/test_configtypes.py @@ -1378,8 +1378,7 @@ class TestFile: os_path.expanduser.side_effect = lambda x: x os_path.isfile.return_value = True os_path.isabs.return_value = False - with pytest.raises(configexc.ValidationError): - self.t.validate('foobar') + self.t.validate('foobar') def test_validate_expanduser(self, os_path): """Test if validate expands the user correctly.""" From 4e61a6123e3bf074c122bba2de9311d14d2290b9 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Wed, 27 May 2015 12:06:51 +0200 Subject: [PATCH 09/19] Probably shouldn't include changes to the gitignore in a PR --- .gitignore | 2 -- 1 file changed, 2 deletions(-) diff --git a/.gitignore b/.gitignore index 9552dc3c4..f3ff3652a 100644 --- a/.gitignore +++ b/.gitignore @@ -22,5 +22,3 @@ __pycache__ /htmlcov /.tox /testresults.html -tags -*.swp From cfae36c5c82d7ec95b67f7c8f0a1db76fea1e8f1 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Wed, 27 May 2015 14:05:29 +0200 Subject: [PATCH 10/19] Adjust name and doc of modified test --- tests/config/test_configtypes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/config/test_configtypes.py b/tests/config/test_configtypes.py index c515c6cef..d8af6995d 100644 --- a/tests/config/test_configtypes.py +++ b/tests/config/test_configtypes.py @@ -1373,8 +1373,8 @@ class TestFile: os_path.isabs.return_value = True self.t.validate('foobar') - def test_validate_exists_not_abs(self, os_path): - """Test validate with a file which does exist but is not absolute.""" + def test_validate_exists_rel(self, os_path): + """Test validate with a relative path to an existing file.""" os_path.expanduser.side_effect = lambda x: x os_path.isfile.return_value = True os_path.isabs.return_value = False From e12dce9d55eb3d1d3aeb56691e1b10fa28c5d506 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Wed, 27 May 2015 14:40:07 +0200 Subject: [PATCH 11/19] Include expandvars in File.transform, adjust test --- qutebrowser/config/configtypes.py | 8 +++----- tests/config/test_configtypes.py | 1 + 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 4b0b5b70f..626ec43c2 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -803,6 +803,7 @@ class File(BaseType): if not value: return None value = os.path.expanduser(value) + value = os.path.expandvars(value) if not os.path.isabs(value): if standarddir.config(): abspath = os.path.join(standarddir.config(), value) @@ -818,10 +819,8 @@ class File(BaseType): raise configexc.ValidationError(value, "may not be empty!") value = os.path.expanduser(value) try: - if not os.path.isabs(value): - abspath = os.path.join(standarddir.config(), value) - if os.path.isfile(abspath): - return + if os.path.isfile(os.path.join(standarddir.config(), value)): + return if not os.path.isfile(value): raise configexc.ValidationError(value, "must be a valid file!") if not os.path.isabs(value): @@ -1166,7 +1165,6 @@ class UserStyleSheet(File): if not value: return None path = super().transform(value) - path = os.path.expandvars(path) if os.path.isabs(path): return QUrl.fromLocalFile(path) else: diff --git a/tests/config/test_configtypes.py b/tests/config/test_configtypes.py index d8af6995d..c7df07f14 100644 --- a/tests/config/test_configtypes.py +++ b/tests/config/test_configtypes.py @@ -1398,6 +1398,7 @@ class TestFile: def test_transform(self, os_path): """Test transform.""" os_path.expanduser.side_effect = lambda x: x.replace('~', '/home/foo') + os_path.expandvars.side_effect = lambda x: x assert self.t.transform('~/foobar') == '/home/foo/foobar' os_path.expanduser.assert_called_once_with('~/foobar') From 4851a3d4424e61b01b3035ca69b12cbacde1c808 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Wed, 27 May 2015 15:39:58 +0200 Subject: [PATCH 12/19] Replace isabs with exists in transform In UserStyleSheet.transform os.path.isabs was replaced with os.path.exists, a more fitting condition. Accordingly two test cases needed to include mocks for os.path.exists and QUrl.fromLocalFile. --- qutebrowser/config/configtypes.py | 2 +- tests/config/test_configtypes.py | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 626ec43c2..c3815cb6d 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1165,7 +1165,7 @@ class UserStyleSheet(File): if not value: return None path = super().transform(value) - if os.path.isabs(path): + if os.path.exists(path): return QUrl.fromLocalFile(path) else: data = base64.b64encode(value.encode('utf-8')).decode('ascii') diff --git a/tests/config/test_configtypes.py b/tests/config/test_configtypes.py index c7df07f14..345a79d3b 100644 --- a/tests/config/test_configtypes.py +++ b/tests/config/test_configtypes.py @@ -1930,13 +1930,21 @@ class TestUserStyleSheet: """Test transform with an empty value.""" assert self.t.transform('') is None - def test_transform_file(self): + def test_transform_file(self, os_path, mocker): """Test transform with a filename.""" + qurl = mocker.patch('qutebrowser.config.configtypes.QUrl', + autospec=True) + qurl.fromLocalFile.return_value = QUrl("file:///foo/bar") + os_path.exists.return_value = True path = os.path.join(os.path.sep, 'foo', 'bar') assert self.t.transform(path) == QUrl("file:///foo/bar") - def test_transform_file_expandvars(self, monkeypatch): + def test_transform_file_expandvars(self, os_path, monkeypatch, mocker): """Test transform with a filename (expandvars).""" + qurl = mocker.patch('qutebrowser.config.configtypes.QUrl', + autospec=True) + qurl.fromLocalFile.return_value = QUrl("file:///foo/bar") + os_path.exists.return_value = True monkeypatch.setenv('FOO', 'foo') path = os.path.join(os.path.sep, '$FOO', 'bar') assert self.t.transform(path) == QUrl("file:///foo/bar") From b5eea81e2ed389487a8cf5cb15043ca6a8094efb Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Thu, 28 May 2015 12:14:12 +0200 Subject: [PATCH 13/19] Fix File.validate and corresponding tests There were no tests regarding the return value of standarddir.config() and thus it wasn't caught that it returned None in some cases. This is now fixed by checking the return of standdarddir.config before calling it and modifying the corresponding test_validate_exists_rel as well as adding a new test_validate_rel_config_none. --- qutebrowser/config/configtypes.py | 12 +++++++----- tests/config/test_configtypes.py | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index c3815cb6d..edc96d89d 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -819,13 +819,15 @@ class File(BaseType): raise configexc.ValidationError(value, "may not be empty!") value = os.path.expanduser(value) try: - if os.path.isfile(os.path.join(standarddir.config(), value)): - return + if not os.path.isabs(value): + if standarddir.config(): + if os.path.isfile( + os.path.join(standarddir.config(), value)): + return + raise configexc.ValidationError(value, + "must be an absolute path!") if not os.path.isfile(value): raise configexc.ValidationError(value, "must be a valid file!") - if not os.path.isabs(value): - raise configexc.ValidationError( - value, "must be an absolute path!") except UnicodeEncodeError as e: raise configexc.ValidationError(value, e) diff --git a/tests/config/test_configtypes.py b/tests/config/test_configtypes.py index 345a79d3b..1d52afdc8 100644 --- a/tests/config/test_configtypes.py +++ b/tests/config/test_configtypes.py @@ -30,7 +30,7 @@ from PyQt5.QtGui import QColor, QFont from PyQt5.QtNetwork import QNetworkProxy from qutebrowser.config import configtypes, configexc -from qutebrowser.utils import debug, utils +from qutebrowser.utils import debug, utils, standarddir class Font(QFont): @@ -1373,12 +1373,24 @@ class TestFile: os_path.isabs.return_value = True self.t.validate('foobar') - def test_validate_exists_rel(self, os_path): + def test_validate_exists_rel(self, os_path, monkeypatch): """Test validate with a relative path to an existing file.""" + monkeypatch.setattr('qutebrowser.config.configtypes.standarddir.config', + lambda: '/home/foo/.config/') os_path.expanduser.side_effect = lambda x: x os_path.isfile.return_value = True os_path.isabs.return_value = False self.t.validate('foobar') + os_path.join.assert_called_once_with('/home/foo/.config/', + 'foobar') + + def test_validate_rel_config_none(self, os_path, monkeypatch): + """Test with a relative path and standarddir.config returning None.""" + monkeypatch.setattr('qutebrowser.config.configtypes.standarddir.config', + lambda: None) + os_path.isabs.return_value = False + with pytest.raises(configexc.ValidationError): + self.t.validate('foobar') def test_validate_expanduser(self, os_path): """Test if validate expands the user correctly.""" From f5d299d8c7170061406fa89020183a2762a2e1ef Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Thu, 28 May 2015 13:05:12 +0200 Subject: [PATCH 14/19] Fix intents --- qutebrowser/config/configtypes.py | 10 +++++----- tests/config/test_configtypes.py | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index edc96d89d..489dc456d 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -820,12 +820,12 @@ class File(BaseType): value = os.path.expanduser(value) try: if not os.path.isabs(value): - if standarddir.config(): - if os.path.isfile( - os.path.join(standarddir.config(), value)): - return + cfgdir = standarddir.config() + if cfgdir: + if os.path.isfile(os.path.join(cfgdir, value)): + return raise configexc.ValidationError(value, - "must be an absolute path!") + "must be an absolute path!") if not os.path.isfile(value): raise configexc.ValidationError(value, "must be a valid file!") except UnicodeEncodeError as e: diff --git a/tests/config/test_configtypes.py b/tests/config/test_configtypes.py index 1d52afdc8..317acbe5e 100644 --- a/tests/config/test_configtypes.py +++ b/tests/config/test_configtypes.py @@ -30,7 +30,7 @@ from PyQt5.QtGui import QColor, QFont from PyQt5.QtNetwork import QNetworkProxy from qutebrowser.config import configtypes, configexc -from qutebrowser.utils import debug, utils, standarddir +from qutebrowser.utils import debug, utils class Font(QFont): @@ -1375,19 +1375,19 @@ class TestFile: def test_validate_exists_rel(self, os_path, monkeypatch): """Test validate with a relative path to an existing file.""" - monkeypatch.setattr('qutebrowser.config.configtypes.standarddir.config', - lambda: '/home/foo/.config/') + monkeypatch.setattr( + 'qutebrowser.config.configtypes.standarddir.config', + lambda: '/home/foo/.config/') os_path.expanduser.side_effect = lambda x: x os_path.isfile.return_value = True os_path.isabs.return_value = False self.t.validate('foobar') - os_path.join.assert_called_once_with('/home/foo/.config/', - 'foobar') + os_path.join.assert_called_once_with('/home/foo/.config/', 'foobar') def test_validate_rel_config_none(self, os_path, monkeypatch): """Test with a relative path and standarddir.config returning None.""" - monkeypatch.setattr('qutebrowser.config.configtypes.standarddir.config', - lambda: None) + monkeypatch.setattr( + 'qutebrowser.config.configtypes.standarddir.config', lambda: None) os_path.isabs.return_value = False with pytest.raises(configexc.ValidationError): self.t.validate('foobar') From 63c9e6a444f1a45a8970e3bfebc2c131bc1c9827 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Thu, 28 May 2015 13:20:00 +0200 Subject: [PATCH 15/19] Another indentation-related fix --- qutebrowser/config/configtypes.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 489dc456d..94bc278bf 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -821,9 +821,8 @@ class File(BaseType): try: if not os.path.isabs(value): cfgdir = standarddir.config() - if cfgdir: - if os.path.isfile(os.path.join(cfgdir, value)): - return + if cfgdir and os.path.isfile(os.path.join(cfgdir, value)): + return raise configexc.ValidationError(value, "must be an absolute path!") if not os.path.isfile(value): From de0686c50ab0a337ba94315a0c3a54ab97499e86 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Sat, 6 Jun 2015 14:04:45 +0200 Subject: [PATCH 16/19] Error messages and explicit test for None Error messages for validate() are more specific. Return of standarddir.conf() is explicitly tested for None to avoid ambiguity with other falsey values. --- qutebrowser/config/configtypes.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 267ae8789..ef23cbbbe 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -805,7 +805,7 @@ class File(BaseType): value = os.path.expanduser(value) value = os.path.expandvars(value) if not os.path.isabs(value): - if standarddir.config(): + if standarddir.config() is not None: abspath = os.path.join(standarddir.config(), value) if os.path.isfile(abspath): return abspath @@ -821,12 +821,16 @@ class File(BaseType): try: if not os.path.isabs(value): cfgdir = standarddir.config() - if cfgdir and os.path.isfile(os.path.join(cfgdir, value)): + if cfgdir is not None and os.path.isfile( + os.path.join(cfgdir, value)): return - raise configexc.ValidationError(value, - "must be an absolute path!") - if not os.path.isfile(value): - raise configexc.ValidationError(value, "must be a valid file!") + raise configexc.ValidationError( + value, "must be an absolute path when not using a config " + "directory!") + elif not os.path.isfile(value): + raise configexc.ValidationError( + value, "must be a valid path relative to the config " + "directory!") except UnicodeEncodeError as e: raise configexc.ValidationError(value, e) @@ -1195,7 +1199,7 @@ class UserStyleSheet(File): raise configexc.ValidationError(value, str(e)) return except UnicodeEncodeError as e: - raise configexc.ValidationError(value, e) + raise configexc.ValidationError(value, str(e)) class AutoSearch(BaseType): From 5bacbc9d3862c7b8225dd9ad3416a35b5fb6a090 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Sat, 6 Jun 2015 14:07:57 +0200 Subject: [PATCH 17/19] Remove obsolete try-except block --- qutebrowser/config/configtypes.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index ef23cbbbe..3b623e3ab 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1193,11 +1193,7 @@ class UserStyleSheet(File): # FIXME We just try if it is encodable, maybe we should # validate CSS? # https://github.com/The-Compiler/qutebrowser/issues/115 - try: - value.encode('utf-8') - except UnicodeEncodeError as e: - raise configexc.ValidationError(value, str(e)) - return + value.encode('utf-8') except UnicodeEncodeError as e: raise configexc.ValidationError(value, str(e)) From 0e50760b70fba54a0af460d66ec6842279e7ab1f Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Mon, 8 Jun 2015 12:53:59 +0200 Subject: [PATCH 18/19] Differentiate exceptions; remove obsolete test In function File.validate the try-except block has been re-written to differentiate raised errors. In function File.transform there was a check for validity of the file path that is alraedy performed by File.validate under the same conditions. This check has been removed. --- qutebrowser/config/configtypes.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 3b623e3ab..96882fa2d 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -805,10 +805,9 @@ class File(BaseType): value = os.path.expanduser(value) value = os.path.expandvars(value) if not os.path.isabs(value): - if standarddir.config() is not None: - abspath = os.path.join(standarddir.config(), value) - if os.path.isfile(abspath): - return abspath + cfgdir = standarddir.config() + if cfgdir is not None: + return os.path.join(cfgdir, value) return value def validate(self, value): @@ -821,16 +820,19 @@ class File(BaseType): try: if not os.path.isabs(value): cfgdir = standarddir.config() - if cfgdir is not None and os.path.isfile( - os.path.join(cfgdir, value)): + if cfgdir is None: + raise configexc.ValidationError( + value, "must be an absolute path when not using a " + "config directory!") + elif not os.path.isfile(os.path.join(cfgdir, value)): + raise configexc.ValidationError( + value, "must be a valid path relative to the config " + "directory!") + else: return - raise configexc.ValidationError( - value, "must be an absolute path when not using a config " - "directory!") elif not os.path.isfile(value): raise configexc.ValidationError( - value, "must be a valid path relative to the config " - "directory!") + value, "must be a valid file!") except UnicodeEncodeError as e: raise configexc.ValidationError(value, e) From 7f27c183beb627962358f7d552b31cd680b9f3b7 Mon Sep 17 00:00:00 2001 From: Lamar Pavel Date: Mon, 8 Jun 2015 13:18:16 +0200 Subject: [PATCH 19/19] Include expandvars in File.validate I thought I put this in here before, but apparently I did not. So here it is, together with a new test to verify it. Other tests needed to be updated with a mock for os.path.expandvars. --- qutebrowser/config/configtypes.py | 1 + tests/config/test_configtypes.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 96882fa2d..ecd11b81a 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -817,6 +817,7 @@ class File(BaseType): else: raise configexc.ValidationError(value, "may not be empty!") value = os.path.expanduser(value) + value = os.path.expandvars(value) try: if not os.path.isabs(value): cfgdir = standarddir.config() diff --git a/tests/config/test_configtypes.py b/tests/config/test_configtypes.py index 317acbe5e..6b6a08594 100644 --- a/tests/config/test_configtypes.py +++ b/tests/config/test_configtypes.py @@ -1362,6 +1362,7 @@ class TestFile: def test_validate_does_not_exist(self, os_path): """Test validate with a file which does not exist.""" os_path.expanduser.side_effect = lambda x: x + os_path.expandvars.side_effect = lambda x: x os_path.isfile.return_value = False with pytest.raises(configexc.ValidationError): self.t.validate('foobar') @@ -1369,6 +1370,7 @@ class TestFile: def test_validate_exists_abs(self, os_path): """Test validate with a file which does exist.""" os_path.expanduser.side_effect = lambda x: x + os_path.expandvars.side_effect = lambda x: x os_path.isfile.return_value = True os_path.isabs.return_value = True self.t.validate('foobar') @@ -1379,6 +1381,7 @@ class TestFile: 'qutebrowser.config.configtypes.standarddir.config', lambda: '/home/foo/.config/') os_path.expanduser.side_effect = lambda x: x + os_path.expandvars.side_effect = lambda x: x os_path.isfile.return_value = True os_path.isabs.return_value = False self.t.validate('foobar') @@ -1395,11 +1398,22 @@ class TestFile: def test_validate_expanduser(self, os_path): """Test if validate expands the user correctly.""" os_path.expanduser.side_effect = lambda x: x.replace('~', '/home/foo') + os_path.expandvars.side_effect = lambda x: x os_path.isfile.side_effect = lambda path: path == '/home/foo/foobar' os_path.isabs.return_value = True self.t.validate('~/foobar') os_path.expanduser.assert_called_once_with('~/foobar') + def test_validate_expandvars(self, os_path): + """Test if validate expands the environment vars correctly.""" + os_path.expanduser.side_effect = lambda x: x + os_path.expandvars.side_effect = lambda x: x.replace( + '$HOME', '/home/foo') + os_path.isfile.side_effect = lambda path: path == '/home/foo/foobar' + os_path.isabs.return_value = True + self.t.validate('$HOME/foobar') + os_path.expandvars.assert_called_once_with('$HOME/foobar') + def test_validate_invalid_encoding(self, os_path, unicode_encode_err): """Test validate with an invalid encoding, e.g. LC_ALL=C.""" os_path.isfile.side_effect = unicode_encode_err