Optimize the naming algorithm in scopes

[aivanoved]

Solves #140

Checklist

• [x] Added a CHANGELOG.rst entry

[jbachurski]

Unsure how much this will change from draft, but I don't like these aspects:

• Introduction of a new class, with an ABC despite there being one implementation and this being an internal utility.
• Abusing Python's iterator interface just to write next(suffix) vs suffix.next().
• The code changed more than it seems necessary - preferably this just addresses #140 algorithmically as outlined by the issue, and then a refactor can be done if that's helpful.
• This moves the dictionary into integers I proposed in #140 into another dictionary into ref-cells of integers, plus there's a redundancy where the Suffix class stores the name and the dictionary indexes by those names. A bunch of abstractions that don't feel useful right now.

Please consider just using a simple map like I proposed there, unless my attempt was wrong :)

[aivanoved]

Thank you for the quick review,

I understand where your concerns come from. In fact, while deciding on whether this is the approach I wanted to take, I did share some of them. I'll be addressing them in what I believe is reverse order of severity

Point 3

The code changed more than it seems necessary - preferably this just addresses Naming algorithm has quadratic time complexity #140 algorithmically as outlined by the issue, and then a refactor can be done if that's helpful.

This is a consequence of all the other points so, if we implement them in the way suggested by the review I believe it will be automatically solved. Nonetheless, this is valid criticism as needless code changes and technical debt are definitely not a great idea

I do however still think, this change is not unnecessary in the way it is currently written in the pr

Point 2

Abusing Python's iterator interface just to write next(suffix) vs suffix.next()

I don't think this is abuse of the interface. What we are creating is a generator of suffixes and is by that definition an iterator. In fact I see the use of suffix.next() as even more problematic as it does not imply interior mutation of the Suffix object, whereas next(suffix) does.

Point 1

Introduction of a new class, with an ABC despite there being one implementation and this being an internal utility.

The introduction of a base class is purely for typing and clarity purposes, it is essentially an interface nothing more than a marker. The reason we need this marker class (yes, we can definitely argue this might be too dogmatic of an OOP approach here) is that, otherwise there is no other clear way of specifying what type of objects are acceptable to be passed to the enum and maybe_enum functions (Iterator[str] is an option, but that does not carry enough information in my opinion to describe the idea of that object)

We need a base/marker class if we want to be able to pass different Suffix classes to the function not just the default one. If we make it just one class then the argument of suffix becomes obsolete. This is definitely a simplifying approach, and one which I would be in favour of, despite the internal utility (Scope.enum and Scope.maybe_enum) interface change.

Point 4

This moves the dictionary into integers I proposed in Naming algorithm has quadratic time complexity #140 into another dictionary into ref-cells of integers, plus there's a redundancy where the Suffix class stores the name and the dictionary indexes by those names. A bunch of abstractions that don't feel useful right now.

I am a little confused as to what you mean in the second to last sentence here.

~~In the proposal in #140 for each base name we put into the memo_enum dictionary each (base, suffix) tuple as a key, here we have the dictionary storing only the one and only Suffix iterator object for the base name in all scopes, current or parent, so in fact there is much less redundancy by the approach in this pr~~

The statement above is incorrect, and after rereading the proposal in #140, it is clear that there is the same amount of information stored in the dictionaries. Note that the change here is more to do with where this information is stored. In the proposal in #140, we have a base name, suffix tuple as a key, whereas here the suffix format is part of the value, both store the index in the value.

Note on the proposal in #140

In any case, I understand why this might seem over engineered. I would not be opposed to implementing the algo as proposed in #140