PR #2826 merged into devel

[Edward K. Ream]

PR #2826 greatly simplifies how Leo handles text indices.

This PR is a mass update. I have carefully compared diffs, all tests pass, mypy and pylint are happy, and I have been eating my own dog food without upset. Nevertheless, the PR may have introduced new bugs. Please report any problems immediately.

The PR contains a high-level summary in the first comment. However, the changes do not capture my excitement about the changes:

1. This PR took about 24 hours. This incredible productivity is the direct result of mypy checks. At last we are reaping the benefit of strong type checking.

2. Usually I have some notion of what the final code will look like. Not this time! Here is the list of the happy surprises:

- All indices everywhere are ints, not a Union.

- Leo's core knows nothing about g.toPythonIndex.

- g.toPythonIndex takes a string argument, not a Union!

- All calls to g.toPythonIndex occur within toInt local helper functions.

- It was easy to delete all the faux-helper x.toPythonIndex methods.

Testing

The PR removes the recently-introduced int_i, int_j, etc. vars. I have reviewed various kinds of diffs, including git-diff-pr between the 6.6.4 and ekr-index branches. What stands out are the many missing calls to g.toPythonIndex.

However, Leo's unit tests don't cover qt_text.py. It would be possible to do so, but not straightforward.  See #2827. In their absence I have relied on mypy, pylint, and code reviews. It's not enough :-)

Summary

The new code base is a spectacular simplification. We'll deal with bugs as they arise.

The difference between Union[int, str] and just int is hard to overstate. The new code should help the leoJS project considerably.

Edward

[spike]

On 2022-09-10 16:39, Edward K. Ream wrote:
> PR #2826 <https://github.com/leo-editor/leo-editor/pull/2828/> greatly

> simplifies how Leo handles text indices.
>
> This PR is a mass update. I have carefully compared diffs, all tests pass,
> mypy and pylint are happy, and I have been eating my own dog food without
> upset. Nevertheless, the PR may have introduced new bugs. Please report any
> problems immediately.
>
> The PR contains a high-level summary in the first comment. However, the
> changes do not capture my excitement about the changes:
>
> 1. This PR took about 24 hours. This incredible productivity is the direct
> result of mypy checks. At last we are reaping the benefit of strong type
> checking.
>
> 2. Usually I have some notion of what the final code will look like. Not
> this time! Here is the list of the happy surprises:
>

> - All indices *everywhere* are ints, not a Union.
> - Leo's core knows *nothing* about g.toPythonIndex.
> - g.toPythonIndex takes a *string* argument, not a Union!

> - All calls to g.toPythonIndex occur within toInt local helper functions.

> - It was *easy* to delete all the faux-helper x.toPythonIndex *methods.*
>
> *Testing*

>
> The PR removes the recently-introduced int_i, int_j, etc. vars. I have
> reviewed various kinds of diffs, including git-diff-pr between the 6.6.4
> and ekr-index branches. What stands out are the many missing calls to
> g.toPythonIndex.
>
> However, Leo's unit tests don't cover qt_text.py. It would be possible to
> do so, but not straightforward. See #2827

> <https://github.com/leo-editor/leo-editor/issues/2827>. In their absence I

> have relied on mypy, pylint, and code reviews. It's not enough :-)
>

> *Summary*

>
> The new code base is a spectacular simplification. We'll deal with bugs as
> they arise.
>
> The difference between Union[int, str] and just int is hard to overstate.
> The new code should help the leoJS project considerably.
>
> Edward
>

Looks good. Will try it out.

[Edward K. Ream]

On Sat, Sep 10, 2022 at 6:10 PM spike <spiketheh...@runbox.com> wrote:

>> PR #2826 <https://github.com/leo-editor/leo-editor/pull/2828/> greatly simplifies how Leo handles text indices.

> Looks good. Will try it out.

Thanks for your testing!

Edward