Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Tim Bentley 
Review: Approve

Looks good to me.
-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343465
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna 
Simon Hanna has proposed merging lp:~thelinuxguy/openlp/fix-newline-bug into 
lp:openlp.

Requested reviews:
  Phill (phill-ridout)
  Tim Bentley (trb143)
Related bugs:
  Bug #1727517 in OpenLP: "Unicode control chars causes song importer to crash"
  https://bugs.launchpad.net/openlp/+bug/1727517

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343465

Fix the fix for 1727517
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.
=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2018-02-24 16:10:02 +
+++ openlp/core/common/__init__.py	2018-04-17 19:37:10 +
@@ -44,7 +44,7 @@
 
 FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
 SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
-CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
+CONTROL_CHARS = re.compile(r'[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]')
 INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
 IMAGES_FILTER = None
 REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', '\u201c': '"', '\u201d': '"', '\u2026': '...',
@@ -471,15 +471,15 @@
 log.exception('Error detecting file encoding')
 
 
-def normalize_str(irreg_str):
+def normalize_str(irregular_string):
 """
 Normalize the supplied string. Remove unicode control chars and tidy up white space.
 
-:param str irreg_str: The string to normalize.
+:param str irregular_string: The string to normalize.
 :return: The normalized string
 :rtype: str
 """
-irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
-irreg_str = CONTROL_CHARS.sub('', irreg_str)
-irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
-return WHITESPACE_REGEX.sub(' ', irreg_str)
+irregular_string = irregular_string.translate(REPLACMENT_CHARS_MAP)
+irregular_string = CONTROL_CHARS.sub('', irregular_string)
+irregular_string = NEW_LINE_REGEX.sub('\n', irregular_string)
+return WHITESPACE_REGEX.sub(' ', irregular_string)

=== modified file 'tests/functional/openlp_core/common/test_common.py'
--- tests/functional/openlp_core/common/test_common.py	2017-12-29 09:15:48 +
+++ tests/functional/openlp_core/common/test_common.py	2018-04-17 19:37:10 +
@@ -25,8 +25,8 @@
 from unittest import TestCase
 from unittest.mock import MagicMock, call, patch
 
-from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, is_win, \
-path_to_module, trace_error_handler
+from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, \
+is_win, normalize_str, path_to_module, trace_error_handler
 from openlp.core.common.path import Path
 
 
@@ -211,6 +211,30 @@
 assert is_win() is False, 'is_win() should return False'
 assert is_macosx() is False, 'is_macosx() should return False'
 
+def test_normalize_str_leaves_newlines(self):
+# GIVEN: a string containing newlines
+string = 'something\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: string is unchanged
+assert normalized_string == string
+
+def test_normalize_str_removes_null_byte(self):
+# GIVEN: a string containing a null byte
+string = 'somet\x00hing'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: nullbyte is removed
+assert normalized_string == 'something'
+
+def test_normalize_str_replaces_crlf_with_lf(self):
+# GIVEN: a string containing crlf
+string = 'something\r\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: crlf is replaced with lf
+assert normalized_string == 'something\nelse'
+
 def test_clean_button_text(self):
 """
 Test the clean_button_text() function.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna 
The proposal to merge lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp has 
been updated.

Status: Needs review => Superseded

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343422
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Tim Bentley 
Review: Needs Fixing

See comments inline

lp:~thelinuxguy/openlp/fix-newline-bug (revision 2822)
https://ci.openlp.io/job/Branch-01-Pull/2502/  [SUCCESS]
https://ci.openlp.io/job/Branch-02a-Linux-Tests/2403/  [SUCCESS]
https://ci.openlp.io/job/Branch-02b-macOS-Tests/189/   [FAILURE]
https://ci.openlp.io/job/Branch-03a-Build-Source/101/  [SUCCESS]
https://ci.openlp.io/job/Branch-03b-Build-macOS/94/[SUCCESS]
https://ci.openlp.io/job/Branch-04a-Code-Analysis/1563/[SUCCESS]
https://ci.openlp.io/job/Branch-04b-Test-Coverage/1376/[SUCCESS]


Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py2018-02-24 16:10:02 +
> +++ openlp/core/common/__init__.py2018-04-17 10:28:19 +
> @@ -471,15 +471,15 @@
>  log.exception('Error detecting file encoding')
>  
>  
> -def normalize_str(irreg_str):
> +def normalize_str(string):

Field names of string are not meaningful.  irreg_string is more correct

>  """
>  Normalize the supplied string. Remove unicode control chars and tidy up 
> white space.
>  
> -:param str irreg_str: The string to normalize.
> +:param str string: The string to normalize.
>  :return: The normalized string
>  :rtype: str
>  """
> -irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
> -irreg_str = CONTROL_CHARS.sub('', irreg_str)
> -irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
> -return WHITESPACE_REGEX.sub(' ', irreg_str)
> +string = string.translate(REPLACMENT_CHARS_MAP)
> +string = CONTROL_CHARS.sub('', string)
> +string = NEW_LINE_REGEX.sub('\n', string)
> +return WHITESPACE_REGEX.sub(' ', string)


-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343422
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna 
Simon Hanna has proposed merging lp:~thelinuxguy/openlp/fix-newline-bug into 
lp:openlp.

Requested reviews:
  Phill (phill-ridout)
  Tim Bentley (trb143)
Related bugs:
  Bug #1727517 in OpenLP: "Unicode control chars causes song importer to crash"
  https://bugs.launchpad.net/openlp/+bug/1727517

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343422

Fix the fix for 1727517
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.
=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2018-02-24 16:10:02 +
+++ openlp/core/common/__init__.py	2018-04-17 10:28:19 +
@@ -44,7 +44,7 @@
 
 FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
 SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
-CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
+CONTROL_CHARS = re.compile(r'[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]')
 INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
 IMAGES_FILTER = None
 REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', '\u201c': '"', '\u201d': '"', '\u2026': '...',
@@ -471,15 +471,15 @@
 log.exception('Error detecting file encoding')
 
 
-def normalize_str(irreg_str):
+def normalize_str(string):
 """
 Normalize the supplied string. Remove unicode control chars and tidy up white space.
 
-:param str irreg_str: The string to normalize.
+:param str string: The string to normalize.
 :return: The normalized string
 :rtype: str
 """
-irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
-irreg_str = CONTROL_CHARS.sub('', irreg_str)
-irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
-return WHITESPACE_REGEX.sub(' ', irreg_str)
+string = string.translate(REPLACMENT_CHARS_MAP)
+string = CONTROL_CHARS.sub('', string)
+string = NEW_LINE_REGEX.sub('\n', string)
+return WHITESPACE_REGEX.sub(' ', string)

=== modified file 'tests/functional/openlp_core/common/test_common.py'
--- tests/functional/openlp_core/common/test_common.py	2017-12-29 09:15:48 +
+++ tests/functional/openlp_core/common/test_common.py	2018-04-17 10:28:19 +
@@ -25,8 +25,8 @@
 from unittest import TestCase
 from unittest.mock import MagicMock, call, patch
 
-from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, is_win, \
-path_to_module, trace_error_handler
+from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, \
+is_win, normalize_str, path_to_module, trace_error_handler
 from openlp.core.common.path import Path
 
 
@@ -211,6 +211,30 @@
 assert is_win() is False, 'is_win() should return False'
 assert is_macosx() is False, 'is_macosx() should return False'
 
+def test_normalize_str_leaves_newlines(self):
+# GIVEN: a string containing newlines
+string = 'something\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: string is unchanged
+assert normalized_string == string
+
+def test_normalize_str_removes_null_byte(self):
+# GIVEN: a string containing a null byte
+string = 'somet\x00hing'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: nullbyte is removed
+assert normalized_string == 'something'
+
+def test_normalize_str_replaces_crlf_with_lf(self):
+# GIVEN: a string containing crlf
+string = 'something\r\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: crlf is replaced with lf
+assert normalized_string == 'something\nelse'
+
 def test_clean_button_text(self):
 """
 Test the clean_button_text() function.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna 
The proposal to merge lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp has 
been updated.

Status: Needs review => Superseded

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343306
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp