openlp-core team mailing list archive

[Andreas Preikschat <andreaspreikschat@xxxxxxxxxx>]

Review: Needs Fixing

- line 844 - 849: you should pass can_shortcuts=True as well

- Constants should be CAPS_SEPARATED_BY_UNDERSCORES
700	+min_fragment_size = 5
701	+min_block_size = 70
702	+max_typo_size = 3

- Line 813: You can put the import on the same line.

- Wouldn't it be better to rework this part, the _length_of_equal_blocks() and _length_of_longest_equal_block() functions?

724	+ if _length_of_equal_blocks(diff_no_typos) >= min_block_size or \
725	+         _length_of_longest_equal_block(diff_no_typos) > len(small) * 2 / 3:
726	+     return True
727	+ else:
728	+     return False

Better:
    return my_function_with_both_checks(diff_no_typos, small)

and:

    def my_function_with_both_checks(diff, smal):
        # Check bla bla bla
        length = 0
        for element in diff:
            if element[0] == "equal":
                new_length = _op_length(element)
                if new_length >= min_block_size:
                    length += new_length
        if length >= min_block_size:
            return True
        # Now check bla bla bla.
        length = 0
        ....
        ....
        ....
        if length > len(small) * 2 / 3:
            return True
        return False

(Maybe you even do not need a new method for this and can just put this in songs_probably_equal())

- You have a conflict (merge trunk and resolve it)
-- 
https://code.launchpad.net/~patrick-zakweb/openlp/duplicate-removal-review/+merge/149724
Your team OpenLP Core is subscribed to branch lp:openlp.