Refactoring suggestins

[frip]

My first shot at a github pull-request.
More may follow...

[Orasund]

Hi, by removing test.sh you have broken the CI pipeline.

The reason for having a generic test.sh is so that the CI pipeline can be language independed. I intend to do more languages besides python

[Orasund]

Hi, please rebase. I've moved the CI code into a ci.sh file.

As far as i can tell adding pyproject.toml broke the CI because now pytest is not included in the requirements.txt

[frip]

Sorry, didn't see your answer in time!
But I found something interesting (I think) in diff_files.py L77ff: did you know, that your get_node() is called with y=-1? (What I am doing here is not stricktly refactoring, because I'm not returning the first element in the line...)

[frip]

This here is my "tricky" fix for p.y=-1. It occurs in the "standard-test-case"!

[Orasund]

I'm a bit puzzled. This is only possible if len(nodes.nodes[0] - 1) = -1 or next_pos.y = -1.

The first case occurs if the second file is empty. Second case occurs if I forgot a guard inside compute_node. In particular, this would mean that the cast is not current.

[Orasund]

In order for next_pos.y = -1 we would need to enter this branch with node1 is None (as there is a guard inside get_node). However, if node1 is None, then either branch 1 or 2 will be entered depending on node1 is None.

So im not seeing what should be wrong here.

[frip]

I saw it in the debugger! :-)

…

On Mon, Apr 22, 2024, 05:26 Orasund ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In diffFiles/src/diff_files.py <#1 (comment)>: > + length = result.nodes[next_pos.x][next_pos.y].length + 1 + else: + candidate1 = Position(i1, i2 - 1) + candidate2 = Position(i1 - 1, i2) + node1 = result.get_node(candidate1) + node2 = result.get_node(candidate2) + + # find the candidate with the bigger length + if node1 is None and node2 is None: + next_pos = None + length = 0 + elif node2 is not None and (node1 is None or node1.length < node2.length): + next_pos = candidate2 + length = node2.length + else: + next_pos = candidate1 In order for next_pos.y = -1 we would need to enter this branch with node1 is None (as there is a guard inside get_node). However, if node1 is None, then either branch 1 or 2 will be entered depending on node1 is None. So im not seeing what should be wrong here. — Reply to this email directly, view it on GitHub <#1 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3LHWLL5KSQ5M63R7NB5ADY6R7PJAVCNFSM6AAAAABGRIUFCWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJTGY4TSNRRGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Orasund]

Thanks a lot. I found the missing guard:

def compute_node(i1: int, i2: int) -> Node:
        next_pos: tuple[int, int] | None
        length: int

        if l1[i1] == l2[i2]:
            if i1 >= 0 and i2 >= 0:
                next_pos = (i1 - 1, i2 - 1)
                length = nodes[next_pos[0]][next_pos[1]].length + 1
            else:
                # this else branch was missing
                next_pos = None
                length = 0

[frip]

I tried adding your guard in the hope that I could then reduce the second closure get_node() to the (new) Matrix-method, but it didn't work... :-(

[Orasund]

Oh! bad luck :/

Hope it's no a dealbreaker. Sadly, I don't have the time to look more into it.