openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Needs Fixing

See in line. One question and a few (more) minor fixes please.

Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py	2018-02-24 16:10:02 +0000
> +++ openlp/core/common/__init__.py	2018-04-14 20:05:07 +0000
> @@ -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-\08\x0E-\x1F\x7F-\x9F]')

is there any reason for removing 09, 0B, 0C (horizontal and vertical tabs as well as form feed)?

>  INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
>  IMAGES_FILTER = None
>  REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', '\u201c': '"', '\u201d': '"', '\u2026': '...',
> 
> === modified file 'tests/functional/openlp_core/common/test_common.py'
> --- tests/functional/openlp_core/common/test_common.py	2017-12-29 09:15:48 +0000
> +++ tests/functional/openlp_core/common/test_common.py	2018-04-14 20:05:07 +0000
> @@ -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)

can you remove the brackets from the import statement please, its not our style.

>  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
> +        str = 'something\nelse'

str is a built-in function, please use another name

> +        # WHEN: normalize is called
> +        normalized_string = normalize_str(str)
> +        # THEN: string is unchanged
> +        assert normalized_string == str
> +
> +    def test_normalize_str_removes_null_byte(self):
> +        # GIVEN: a string containing newlines
> +        str = 'somet\x00hing'

and here

> +        # WHEN: normalize is called
> +        normalized_string = normalize_str(str)
> +        # THEN: string is unchanged
> +        assert normalized_string == 'something'
> +
> +    def test_normalize_str_replaces_crlf_with_lf(self):
> +        # GIVEN: a string containing crlf
> +        str = 'something\r\nelse'

here too!

> +        # WHEN: normalize is called
> +        normalized_string = normalize_str(str)
> +        # THEN: crlf is replaced with lf
> +        assert normalized_string == 'something\nelse'
> +
>      def test_clean_button_text(self):
>          """
>          Test the clean_button_text() function.


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