[Cuis-dev] Recent changes to Encoder (update 4156?)

Tue Jun 2 06:08:43 PDT 2020

Hi Phil,

On Mon, Jun 1, 2020 at 5:13 PM Phil B <pbpublist at gmail.com> wrote:

> Hernan,
>
> Since you are changing both the content and behavior, I'm not sure test
> cases would be much help.
>

oh yes it would help! If we make a change and see a test failing we have to
see why and understand the reason, etc.
For example, if we would have run your tests after this change and some of
them would have failed, we would have contacted you about it before pushing
the change and avoid any bad surprise

> It's the ongoing shifting around without understanding where things are
> going to land that's causing me pain.  What would make things easier on me
> is, to the extent possible, in one set of changes get the external
> interfaces looking and behaving from the outside the way you want them long
> term... regardless of what's going on internally.  I'd rather see one set
> of changes that break everything at once than 5 smaller sets that break it
> a little at a time.
>
> In the short term, that might mean renaming any methods you want now.  On
> the methods that you expect to accept/return a collection of Interval, put
> asserts to that effect.  The same thing for any methods you expect to
> accept/return singular Interval.  That way it's clear what is expected
> where and you are free to do whatever you need to behind the curtains.
> When you get to the final desired end state, and assuming everything is
> using a unified set of assumptions/interfaces, then just drop the asserts
> and any obsolete methods.
>

I understand your point but it is difficult to know how everybody is using
the system. When I added the change I thought it was harmless because it
added range information on nodes that where not in the ranges dictionary.
In fact, no Cuis tool broke. Because I did not know you were using the
ranges information in a different way it is used by the Cuis tools is that
I could not prevent this change from breaking what you have.
If I would have run tests related to what you do, and those test would have
failed, the history would have been different.
If you have tests to run on the packages you are developing please let me
know to add them when we make this kind of changes.

> On the other hand, if the plan is to have the nodes keep track of the
> ranges, do the recent changes even help you get there?  If not, why are you
> bothering to clean up something that's going to go away? (That's creating
> more work for both you and me... and I'm really lazy so that bums me out 😂
>

Well you know that ideas do not pop up in the best order :-)
And as you may also know, we are making all these things on our free time,
that it is scarce, so trade offs between time, impact, change deep and so
on have to be made.

Anyway, I hope we wont break your code again. If you have tests we can run,
that will help a lot.

Cheers!
Hernan.

>
>
> Thanks,
> Phil
>
> On Mon, Jun 1, 2020 at 2:26 PM Hernan Wilkinson <
> hernan.wilkinson at 10pines.com> wrote:
>
>> Hi Phil!
>>  I agree that we should convert the value of the ranges dictionary to a
>> collection of intervals and make them polymorphic once and for all. Sadly
>> that is a change that requires time and great care because it is used in
>> many places (parser, debugger, etc).
>>  I think the best change would be for AST nodes to know their range and
>> get rid of that dictionary in the encoder, that is something that Nahuel
>> has been analyzing lately but again, it is not an easy change.
>>
>> I would like to avoid the problems we are generating to you with these
>> changes... are there tests we can run to be sure we have not break you
>> anything?
>>
>> Cheers!
>> Hernan.
>>
>>
>> On Thu, May 28, 2020 at 3:48 AM Phil B via Cuis-dev <
>> cuis-dev at lists.cuis.st> wrote:
>>
>>> Nahuel,
>>>
>>> Thanks for the response... it gave me a chance to calm down and take a
>>> closer look at what you changed...
>>>
>>> On Wed, May 27, 2020 at 5:51 PM Nahuel Garbezza <n.garbezza at gmail.com>
>>> wrote:
>>>
>>>> Hi Phil,
>>>>
>>>> As you were mentioning, the update #4156 enforces that
>>>> #noteSourceRange:forNode: should always receive an interval instead of a
>>>> collection. Before this update it worked with both intervals and collection
>>>> of intervals, kind of a "weird polymorphism" case (because the object was
>>>> only put into a dictionary, we didn't have any issues). When I looked at
>>>> all the senders of that message, I saw that we are always passing a single
>>>> range instead of a collection, that's why I went ahead with the change.
>>>>
>>>
>>> My issue with these changes since they were originally made was that the
>>> interface and internal representation, while simple, are rather ugly IMO.
>>> It looks like your changes are addressing the ambiguous interface (to a
>>> degree) but not the internal representation, which senders must still be
>>> aware of.  My code being outside of the changesets gets to see the ugliness
>>> first hand...
>>>
>>>
>>>>
>>>> I'm not sure if I understood correctly the issue you are having. Have
>>>> you tried using #addMultiRange:for: message? this one is for the nodes that
>>>> should have collections of source ranges (the nodes that have multiple
>>>> occurrences on the same AST, like variables). The reason for having two
>>>> messages for adding source ranges is for the user to explicitly decide when
>>>> to report a multi range vs. report a single range. Another option could be
>>>> to have a message like #noteSourceRanges:forNode: (note the plural) to be
>>>> used with a collection of source ranges, and report all source ranges at
>>>> once.
>>>>
>>>
>>> The issue is the parallel, yet incompatible, accessors of the same ivar
>>> (sourceRanges) depending on whether you want a singular range or a multi
>>> range.  As a result, the sender of the setter and getter must be
>>> coordinated on a per-node basis or you will have problems... which is what
>>> was happening with my code.
>>>
>>>
>>>> Happy to discuss alternatives to consider all possible use cases!
>>>>
>>>
>>> Why not just convert everything to use multi ranges (i.e. internally
>>> *always* store a collection in sourceRanges) and eliminate the issue of
>>> needing to be aware of the internal representation entirely?
>>>
>>>
>>>>
>>>> Thanks,
>>>> Nahuel
>>>>
>>>>
>>> Thanks,
>>> Phil
>>> --
>>> Cuis-dev mailing list
>>> Cuis-dev at lists.cuis.st
>>> https://lists.cuis.st/mailman/listinfo/cuis-dev
>>>
>>
>>
>> --
>>
>> *Hernán WilkinsonAgile Software Development, Teaching & Coaching*
>> *Phone: +54-011*-4893-2057
>> *Twitter: @HernanWilkinson*
>> *site: http://www.10Pines.com <http://www.10pines.com/>*
>> Address: Alem 896, Floor 6, Buenos Aires, Argentina
>>
>

-- 

*Hernán WilkinsonAgile Software Development, Teaching & Coaching*
*Phone: +54-011*-4893-2057
*Twitter: @HernanWilkinson*
*site: http://www.10Pines.com <http://www.10pines.com/>*
Address: Alem 896, Floor 6, Buenos Aires, Argentina
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20200602/9997f3f1/attachment.htm>